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

Refactor APLpy to use WCSAxes #239

Merged
merged 54 commits into from Aug 1, 2017
Merged

Conversation

astrofrog
Copy link
Member

@astrofrog astrofrog commented Feb 15, 2015

This is work by @anizami that was in the wcsaxes-integration branch which I have now rebased.

My plan in the long term is to:

  • release APLpy 1.0 from the current master branch (will happen in next few days). I will then create the v1.x branch and master will then become 2.0.dev (see next item).
  • release APLpy 2.0 with the changes in this PR, but I will make it a hidden release on PyPI which requires using pip install aplpy --pre so that it can get some testing for a while. I also plan to add aplpy.legacy to this branch which will be a frozen version of APLpy 1.0. Then people who really need to keep the old behavior can do from aplpy.legacy import FITSFigure.

Main to-dos for this PR:

  • Documentation that explains the changes
  • Update reference images
  • Include any other changes that can require astropy 1.0 (for example Use astropy.visualization wherever possible #231)
  • Address any remaining #TODOs in code
  • Check test coverage
  • Make sure all the pixel scale stuff is right (use functions from Astropy 1.0)

Latest status

This PR is almost complete. I think the public API should be pretty much backward-compatible,
though the resulting plots will not be (in most cases they look better in the new version).

@keflavich
Copy link
Member

@astrofrog - I'm glad to see wcsaxes being incorporated now. Are any changes to the API planned or expected? Are there known incompatibilities between aplpy-wcsaxes and aplpy-1.0? These things should be linked from this PR and included in the release notes.

@astrofrog
Copy link
Member Author

@keflavich - I don't have answers to all those questions yet, but the idea is to make sure it's 100% backward-compatible in terms of API. However, we can't guarantee that the output will look exactly the same (in fact, some defaults relating to the tick label spacing may be different). I'll let you know once this is ready for testing (I don't think the tests pass yet).

@astrofrog
Copy link
Member Author

The remaining failure is due to the pixel scale stuff, I need to refactor to use the Astropy 1.0 functions.

@astrofrog
Copy link
Member Author

With the latest commit, we're now removing a total of 2200 lines or so!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.88%) to 70.62% when pulling 27c4cc7 on astrofrog:wcsaxes-integration into 07805a9 on aplpy:master.

@astrofrog
Copy link
Member Author

The remaining failure is due to astropy/astropy#3506

@astrofrog
Copy link
Member Author

Note to self: some of the reference images have changed in ways that are not backward-compatible, so need to investigate this.

@astrofrog
Copy link
Member Author

astrofrog commented Jul 30, 2017

@keflavich - I think I'd like to merge this before it grows anymore. You can see the latest documentation here:

http://aplpy-preview.readthedocs.io/en/latest/

Where you'll see I've taken a stab at clarifying the relation between APLpy and WCSAxes.

Do you have any objections to me merging this and then continuing in separate PRs? The test coverage has already gone up a bit, but I'd like to get it closer to 100% if possible to really make sure the API is going to be backward-compatible.

I'll also add aplpy.legacy in a separate PR to not clutter things here.

@keflavich
Copy link
Member

@astrofrog I'm in favor of merging and moving on. In most of my latest work, I've switched over to 100% wcsaxes, but some things (e.g., the spectral-cube quicklooks) still use aplpy. So I can still provide some tests. I won't have time over the next few weeks to give this PR a thorough review, unfortunately.

version=VERSION,
description=DESCRIPTION,
scripts=scripts,
install_requires=['astropy', 'numpy', 'matplotlib'],
provides=[PACKAGENAME],
install_requires=metadata.get('install_requires', 'astropy').strip().split(),
Copy link
Contributor

Choose a reason for hiding this comment

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

you didn't add 'install_requires' to the metadata, isn't mpl in fact required?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, I'll add that, thanks!

@astrofrog astrofrog changed the title WIP: refactor APLpy to use WCSAxes Refactor APLpy to use WCSAxes Aug 1, 2017
@astrofrog astrofrog merged commit eb09ec3 into aplpy:master Aug 1, 2017
@astrofrog
Copy link
Member Author

@keflavich - thanks!

@anizami
Copy link

anizami commented Aug 1, 2017

I know I'm late to the commenting party but I'm so happy to see this merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants