-
Notifications
You must be signed in to change notification settings - Fork 119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend meta_df tests to include datetime #236
Extend meta_df tests to include datetime #236
Conversation
593729d
to
6b5688d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @znicolls for extending the tests - important steps towards full support of datetime
!
one request to add a sanity check on the new `swap_time_for_year()´ function, and some suggestions for a more concise notation in the tests.
Co-authored-by: danielhuppmann <huppmann@iiasa.ac.at>
Co-authored-by: danielhuppmann <huppmann@iiasa.ac.at>
Co-authored-by: danielhuppmann <huppmann@iiasa.ac.at>
Co-authored-by: danielhuppmann <huppmann@iiasa.ac.at>
cheers @danielhuppmann, that should all be addressed now! |
fda4d57
to
dec9eb6
Compare
3b928a9
to
60ccc9a
Compare
not sure what's going on with CI, read through pyproj4/pyproj#134 but am none the wiser... |
sorry about this - @gidden and I already discussed that the region-plotting-tests are quite unstable. How about deactivating those for Windows machines? |
Hi folks - indeed these are our most finicky tests. One suggestion before turning them off for windows. In pyproj4/pyproj#134, someone notes that the issue goes away for pyproj>2.1. On appveyor, I see that the version is:
So perhaps we can try adding the minimum version to our appveyor install? If that still fails, then I'm ok with just turning them off on windows.. |
Hey @znicholls, I'm now digging through more geopandas/cartopy/pyproj issues. I have two thoughts. First is related to geopandas/geopandas#830, which suggests a specific pyproj version for windows. To attempt this, can you please revert the update in Second, I'm looking through our last successful CI on windows: https://ci.appveyor.com/project/gidden/pyam/builds/24523793/job/2idsdm0hhgvg2tfw In reviewing the package versions at runtime, there is no difference between that run and 60ccc9a. In reviewing the installed conda packages, however, I see the following difference: Successful run:
Failed run:
Thus this implies one potential fix is to install I know this is annoying, but if you would be willing to try those two changes, that would be amazing. Otherwise, let's just turn this stuff off for windows. |
0d0d236
to
545e680
Compare
I don't know whether to be happy or sad, but it looks fixed now! |
A few inline comments, then this is g2g for me. @danielhuppmann you still have this blocking in review, so let us know if it passes muster! |
38e75f6
to
b1119c2
Compare
Co-authored-by: gidden <matthew.gidden@gmail.com>
b1119c2
to
5cbcb42
Compare
Looks great! I'll let @danielhuppmann pull this one in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @znicholls
Please confirm that this PR has done the following:
Adding to RELEASE_NOTES.md (remove section after adding to RELEASE_NOTES.md)
Please add a single line in the release notes similar to the following:
Description of PR
Closes #222. Good news is this was already sorted in v0.2.0 so all this does is make sure it stays that way by expanding the tests a bit.