-
Notifications
You must be signed in to change notification settings - Fork 279
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
add support for numpy 1.18.1 #2448
Conversation
Thank you for backporting that! Maybe we should bump the numpy version in |
You're welcome! 🎉 It was a fun learning experience. So we don't set the version for numpy in test_requirements, but I've bumped the version in the travis config file! |
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.
Good job, thanks for fixing this !
I have several very minor suggestions but nothing critical. Although I'm noting that bumping the test version number for numpy on travis broke the py27 tests, for obvious reasons. Is this a good time to just drop py27, testwise ?
Hey Clement, All of the changes to yt.units are backported from unyt and probably shouldn’t be changed to make future backports (if any) easier. |
Oh right, my mistake. I did not realise the nature of the backport. Sorry ! |
Co-Authored-By: Clément Robert <clement.robert@oca.eu>
Ok, I've done the following:
We do have a test job running python 3.5, which is set to be sundown in September. Do we want to remove this job and add a new one for py 3.8? (note we also don't have tests on travis for py3.7 yet either) |
Why keep these? I don’t think we need to have any python2.7 testing at all. Dropping 3.5 and adding 3.7 and 3.8 makes sense to me. That said, I think there only needs to be one minimal dependency version job, with I don’t think there’s enough marginal benefit to justify the additional complication of doing the minimal tests on multiple python versions, especially now that we’ve dropped python2.7 and don’t have to worry about 2/3 compatibility anymore. |
I'm happy to remove them if we decide that's best. I mostly kept them because we could catch backwards incompatibilities with them. But since we're dropping py2 support it makes sense to remove them. If we do, then there will only be two minimal dependency version jobs (for unit tests and answer tests in python3) and the issue with multiple |
The test_ufuncs test is now failing against python 3.6 (minimal deps run), though you clearly bumped the minimal numpy version to 1.18.1... I'm not getting it. |
No, the minimal numpy is 1.12 on 3.6. @munkm it might be easiest to only run these multiple-output ufunc tests on sufficiently new numpy. Another option would be to bump the minimum required numpy to match unyt (1.13), those tests seem to pass for unyt on numpy 1.13. |
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.
all -remaining- tests are now passing, so I'm updating my review's status to "approved". Thanks again !
PR Summary
This should fix the issues reported in #2445 caused by updates in our upstream packages.
For the yt_units errors I followed the same approach taken in yt-project/unyt#115. Thanks @ngoldbaum; your work made this a lot easer for me.
PR Checklist