Skip to content
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

Numpy >= 1.13, matplotlib >=2.0 for astropy 3.1 #7058

Merged
merged 11 commits into from Jun 29, 2018

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Jan 10, 2018

As discussed on the mailing list, for 3.1 we may want to depend on numpy >=1.13, since this allows much better performance for Quantity (and thus coordinates, etc.) because it introduced __array_ufunc__. This PR removes the stuff needed for numpy < 1.13.

Note that with this PR, 3.1 will likely only be compatible with numpy 1.13 and 1.14, so 2 versions, though 1.15 would presumably be around the corner by then.

fixes #7545

@mhvk mhvk added this to the v3.1 milestone Jan 10, 2018
@astropy-bot
Copy link

astropy-bot bot commented Jan 10, 2018

Hi there @mhvk 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@bsipocz
Copy link
Member

bsipocz commented Jan 14, 2018

(a minor note for the above, we can obviously make most efforts to be np 1.15 compatible even if that release is not out, and add any necessary fixes for full compatibility in a 3.1.x bugfix)

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to also update __minimum_numpy_version__ in astropy/__init__.py

CHANGES.rst Outdated
@@ -215,8 +215,7 @@ astropy.wcs
Other Changes and Additions
---------------------------

- Nothing changed yet.

- Versions of Numpy <1.10 are no longer supported. [#7058]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean <1.13?

@mhvk
Copy link
Contributor Author

mhvk commented Jan 18, 2018

OK, thanks, now fixed. I don't know how to change the circleci setup - I think we need a new image? Also, the matplotlib 1.5 failure is odd. It brings up a question of whether we should still support that, or make 3.1 a fairly clean break with old cruft. (E.g., could go for scipy >= 1.0 as well...)

@astrofrog
Copy link
Member

@mhvk - can you rebase? We now have an updated CircleCI configuration, so hopefully this will be fixed.

@mhvk
Copy link
Contributor Author

mhvk commented Feb 7, 2018

@astrofrog - OK, rebased. Hopefully tests still pass!

@astrofrog
Copy link
Member

astrofrog commented Feb 8, 2018

@mhvk - a few small comments:

  • Can you change astropy/image-tests-py35-mpl153:1.0 to astropy/image-tests-py35-mpl153:1.1 in .circleci/config.yml? (I updated Numpy in there). Also,

  • If I try and run python setup.py install, I get the following rather obscure message:

NotImplementedError: __array_wrap__ should not be used with a context any more, since we require numpy >=1.13.  Please raise an issue on https://github.com/astropy/astropy

Can you move:

if not _ASTROPY_SETUP_:
    _check_numpy()

sooner in astropy/__init__.py?

@mhvk mhvk force-pushed the numpy-at-least-1-13 branch 2 times, most recently from 3567f44 to cf51bbe Compare March 7, 2018 17:42
@mhvk
Copy link
Contributor Author

mhvk commented Mar 7, 2018

@astrofrog - I made most of the smaller changes, but do not understand where you would like me to move the numpy check in __init__.py - it already is just after the required part that defines whether or not we're in setup.

@mhvk
Copy link
Contributor Author

mhvk commented Mar 7, 2018

@astrofrog - mpl153 is failing. Do you have any idea? It does seem related to something in numpy.

@@ -14,7 +14,7 @@ jobs:
command: PYTHONHASHSEED=42 /opt/python/cp36-cp36m/bin/python setup.py test --parallel=4
image-tests-mpl153:
docker:
- image: astropy/image-tests-py35-mpl153:1.0
- image: astropy/image-tests-py35-mpl153:1.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhvk - can you try changing to 1.2? I think that should hopefully fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed (merged into last commit).

@mhvk
Copy link
Contributor Author

mhvk commented Mar 10, 2018

@astrofrog - the 153 circle-io file still seems to use numpy < 1.13... I think that failure also explains your earlier comment that it would be nice if the error message was clearer -- somehow get_package_info() seems to require importing more than perhaps it should? I must admit being somewhat at a loss...

@astrofrog
Copy link
Member

I can investigate this during the week of March 19th

@mhvk
Copy link
Contributor Author

mhvk commented Apr 25, 2018

Rebased, in part to see if the mpl153 got magically resolved... (though I still think it is fine to start requiring matplotlib >=2.0 for astropy 3.1)

@mhvk mhvk force-pushed the numpy-at-least-1-13 branch 2 times, most recently from c7894dc to cae0605 Compare April 25, 2018 14:56
@astrofrog
Copy link
Member

I agree about requiring Matplotlib >= 2.0, as 1.5 is reasonably ancient now. The issue is indeed that the docker image uses Numpy 1.10

@mhvk
Copy link
Contributor Author

mhvk commented Jun 14, 2018

@saimn - very strange. The only thing I noticed about it, is that it still uses matplotlib 1.5, which should not be there. Running it again without that, just in case it makes a difference.

@mhvk
Copy link
Contributor Author

mhvk commented Jun 14, 2018

@saimn - it was a matter of assuming dict-like .values() have guaranteed order...

@mhvk
Copy link
Contributor Author

mhvk commented Jun 14, 2018

The coverage failure is, I think, just because quite a number of lines got removed.

@astrofrog - anything remaining here?

@bsipocz
Copy link
Member

bsipocz commented Jun 15, 2018

@mhvk - I agree to ignore coveralls, it complains about dropped coverage in generated c code, but then there is no diff for those, so difficult to do anything really.

@mhvk
Copy link
Contributor Author

mhvk commented Jun 18, 2018

@astrofrog, are you happy with this as is?

@mhvk
Copy link
Contributor Author

mhvk commented Jun 29, 2018

@astrofrog - I looked again at this move to numpy 1.13, and do think it is all OK. Could you have a look or dismiss your review and ask someone else?

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup!

@pllim
Copy link
Member

pllim commented Jun 29, 2018

I'm merging this now to simplify work of integrating with Python 3.7. Thanks!

@pllim pllim merged commit 4bd2394 into astropy:master Jun 29, 2018
@mhvk mhvk deleted the numpy-at-least-1-13 branch June 29, 2018 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove numpy hack in io.fits when Numpy minversion is 1.12
5 participants