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

Replace old coordinate transformation functions from APLpy by Astropy #578

Merged
merged 12 commits into from
Mar 19, 2015

Conversation

astrofrog
Copy link
Member

This requires Astropy 0.4 in order to work.

Note that Astropy now allows us to have many more kinds of coordinate conversion so we may want to try and think of a more general way to handle these. I think this should actually be developed as a plugin really (a default plugin, but a plugin nevertheless).

@astrofrog
Copy link
Member Author

Remaining todo: add conversion accuracy tests (just a couple) to make sure it's all good.

@astrofrog
Copy link
Member Author

I'm going to try and refactor the coordinate conversions as a plugin since they are domain-specific (as a general rule, I guess any domain-specific stuff should be plugin-ized?)

@astrofrog
Copy link
Member Author

This now includes some refactoring into a plugin.

@ChrisBeaumont - a couple of questions:

  • I need to dig deeper into how sessions are saved, but essentially, if we want to preserve backward compatibility, do we need to keep the linker functions, or at least references to them, where they existed before? Or can sessions be re-loaded even if the original functions don't exist anymore?
  • Related to this, do we want to deprecate the single coordinate conversion, e.g radec2glon and so on? We could simply remove them, or add a flag to helpers that indicates whether they are deprecated, and if so we keep them internally but don't display them in the GUI. If we do that I will then be able to add a few more common coordinate conversions (but the list would be unmanageable if we had to create one class and four functions for each possibility)

Once we settle on the above questions, I'll add conversions between the main common systems (ICRS, FK4, FK5, and Galactic).

@astrofrog astrofrog force-pushed the coord-transform branch 2 times, most recently from 5aa68b5 to f3fdac8 Compare February 28, 2015 15:57
@ChrisBeaumont
Copy link
Member

Deprecating sounds fine. They are almost never useful

On Saturday, February 28, 2015, Thomas Robitaille notifications@github.com
wrote:

(just for info the last commit adds an example of how we can deprecate the
functions, they then no longer appear in the list)


Reply to this email directly or view it on GitHub
#578 (comment).


Chris Beaumont
Senior Software Engineer
Harvard Center for Astrophysics
60 Garden Street, MS 42
Cambridge, MA 02138
chrisbeaumont.org


@astrofrog
Copy link
Member Author

I have now deprecated the single functions for coordinate conversions, and added a few more for the class-based helper. I also added the ability to give a helper class a display name for the drop-down menu so it doesn't have to use the class name:

screen shot 2015-02-28 at 8 32 44 pm

Now one downside of the approach I am using to create the link classes is that the result (once glued) looks like:

screen shot 2015-02-28 at 8 29 50 pm

which is ugly because of the 'forward' and 'backward'. In addition to that, I think only two links should be shown because celestial coordinates are always converted in pairs.

I'll need to think about it more but @ChrisBeaumont, if you have any thoughts, let me know!

@astrofrog
Copy link
Member Author

I've now made it so that the name of a PartialFunction includes the class name which I think makes it more readable:

screen shot 2015-03-07 at 12 46 00 pm

I also added accuracy tests for the coordinate transforms.

@ChrisBeaumont - this is now ready for review!

@ChrisBeaumont
Copy link
Member

Looks good!

@astrofrog astrofrog force-pushed the coord-transform branch 2 times, most recently from fe321d2 to 42e7d03 Compare March 18, 2015 22:33
astrofrog added a commit that referenced this pull request Mar 19, 2015
Replace old coordinate transformation functions from APLpy by Astropy
@astrofrog astrofrog merged commit ce54874 into glue-viz:master Mar 19, 2015
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.

2 participants