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

Add more ecliptic frames #8394

Merged
merged 19 commits into from Apr 19, 2019

Conversation

Projects
None yet
7 participants
@Juanlu001
Copy link
Member

commented Feb 3, 2019

As a summary for #6508, there are several conflicting requirements with respect to ecliptic frames:

  • Correctness (apart from the bug in the Sun position arithmetic). #6508 changed the underlying transformation to use the mean ecliptic, but to reduce disruption we kept the names as *TrueEcliptic, which is misleading. This needed some fix.
  • Backwards compatibility. We kind of broke it by changing the nature of the frames, but just renaming them would lead to user code to stop working. For this reason, and also to offer true ecliptic frames just for those who know what are they doing, we wanted to keep the *TrueEcliptic frames.
  • User friendliness. By following the present convention of using a different class for each different model, we end up with six different classes for the ecliptic frames: three different centers and two different nutation models. This does not include models predating IAU 2000/2006 resolutions. Having too many classes can confuse less skilled users.
  • Simplicity (understood as easy implementation). Some users want to specify the obliquity themselves, and also the multiplicity of frames seems to indicate that there should be some way to avoid copy-pasting them in Astropy source and making tiny changes for mean vs true, obliquity, and centers.

As a way to put some solution forward, I took the easy route for this pull request and sacrificed the number of classes in favor of everything else. In short:

  • Makes some fixes to docstrings and transformations that were accidentally overseen in #6508 (see first three commits).
  • Adds new *MeanEcliptic frames that have the correct (mean) transformations (see fourth commit).
  • Changes again the *TrueEcliptic frames to have the correct (true) transformations, as was the case before #6508 (see fifth commit).
  • It adds a new HeliocentricEclipticIAU76 frame with a fixed equinox at J2000 and the IAU 1976/1980 obliquity, for comparison with systems like JPL HORIZONS (inspired by poliastro, see sixth commit).
  • It also adds a new CustomBarycentricEcliptic frame with a fixed equinox at J2000 and a custom obliquity, especially for pulsar timing (inspired by PINT, see seventh commit).

Two main disadvantages:

  • It introduces a lot of code duplication.
  • The number of ecliptic frames ends up being quite high.

Still missing:

  • For the moment I didn't add any tests, but my intention is to at least cover HeliocentricEclipticIAU76 and CustomBarycentricEcliptic. The *TrueEcliptic would necessarily need some hardcoded values for the tests.
  • I didn't touch the documentation, but I suppose we would need warnings or explanations here and there.

I understand that 4.0 will be LTS, so let's think of a roadmap for 3.2 and 4.0 that makes everyone reasonably happy while respecting the expectations in terms of backwards compatibility breakages, correctness, user friendliness and simplicity.

Note: We kept the old idea of adding *CorrectTrueEcliptic in the git history for posterity (see eighth commit).

@mhvk

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

cc @luojing1211 for checking that this would be what can be used in PINT

@luojing1211

This comment has been minimized.

Copy link

commented Feb 3, 2019

@mhvk The CustomBarycentricEcliptic is definatly the type we want. I have two questions,

  1. How does the obliquity gets input when initializing the object? Or does this happen in the level of making the class.
  2. Does ICRS defines its x-axis direction pointing the mean equinox or the ture equinox. Since in pulsar timing, the ecliptic coordinate is just a simple rotation along the ICRS x-axis.

@bsipocz bsipocz added this to the v3.2 milestone Feb 3, 2019

@bsipocz bsipocz requested review from eteq and adrn Feb 3, 2019

@bsipocz

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

It makes some fixes to docstrings and transformations that were accidentally overseen in #6508 (see first three commits).

not critical but these may be factored out to a backportable PR

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2019

@luojing1211 in principle the definition is a copy-paste from PINT, so it should be compatible. The obliquity is entered when instantiating the CustomBarycentricEcliptic class, and the transformation is only a rotation around the X axis of ICRS.

@bsipocz Sure! I arranged the commits to make it easy.

@eteq

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Thanks for the summary and all the work here, @Juanlu001! I haven't had time to dig in detail yet, but a high-level thought to address the code duplication point: what about inserting some abstract classes that provide the common functionality? E.g. an "abstractmeanecliptic" and "abstracttrueecliptic" and then the subclasses just implement the matricies in question or something like that? Or alternatively (although perhaps too confusingly?) the "true" frames can be subclasses of the "mean" frames that add one more rotation?

Either of those might be bigger refactors, and I'm ok with the idea of getting this in first more as it is and adding a follow-on issue to reduce the duplication...

Also FWIW, I don't really consider "The number of ecliptic frames ends up being quite high" to be a disadvantage. If I've learned nothing else from this, it's that there really are a zillion different ecliptic frames different people care about!

@eteq

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Oh and on the deprecation pathway, I think if we can the goal is to leave the results the same for 3.2 but with deprecation warnings for anyone using that pathway, and then maybe in 4.0 remove those/actually rename everything to the more logically-sensible naming scheme?

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

I think if we can the goal is to leave the results the same for 3.2 but with deprecation warnings for anyone using that pathway, and then maybe in 4.0 remove those/actually rename everything to the more logically-sensible naming scheme?

Sounds good, but just to be sure: *TrueEcliptic should return the same result as it does in 3.0 and 3.1, correct? And then the *MeanEcliptic frames would return that same result (but without any warnings), and no changes for the "other" ecliptic frames.

what about inserting some abstract classes that provide the common functionality? E.g. an "abstractmeanecliptic" and "abstracttrueecliptic" and then the subclasses just implement the matricies in question or something like that?

OK, I will try to see how far I can go with this!

Or alternatively (although perhaps too confusingly?) the "true" frames can be subclasses of the "mean" frames that add one more rotation?

I'm not super convinced about this second idea though.

@eteq

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Sounds good, but just to be sure: *TrueEcliptic should return the same result as it does in 3.0 and 3.1, correct? And then the *MeanEcliptic frames would return that same result (but without any warnings), and no changes for the "other" ecliptic frames.

You mean for 3.2, right? yes, that's what I was thinking. But then we'd have *ActuallyNoReallyWeSwearItsReallyTheTrueEcliptic (of course that needs a better name...) as a "temporary" class. Then either we nix *TrueEcliptic entirely or change the temporary class to be*TrueEcliptic.

That way by 3.2 we achieve correctness for those who use the "right" true ecliptic, but still get backwards compatibility until 4.0.

A second option would be to wait until 4.1 to drop the "wrong" TrueEcliptic, but we don't really have to make a decision on that for ~9 months.

@pllim

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

TruerEcliptic? UltimateTruthEcliptic? TrulyEcliptic? DontUseTheOtherOneTrueEcliptic? SupremeTrueEcliptic?

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

I propose CorrectTrueEcliptic and ActualTrueEcliptic. My, I can't wait to drop those 😅

@Juanlu001 Juanlu001 force-pushed the Juanlu001:more-ecliptic-frames branch 2 times, most recently from 9760a06 to 755f09d Feb 28, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

Current status (diff from master):

  • No changes in *TrueEcliptic behavior, although they raise a FutureWarning stating that the semantics will change
  • New *MeanEcliptic classes
  • New *CorrectTrueEcliptic classes (necessary for calc_moon!) that raise a DeprecationWarning stating that they will disappear in the next version
@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@mhvk I admit I'm a bit lazy to start refactoring the classes, but also we plan to drop three of them in the next release, so perhaps it's worth waiting?

@mhvk

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

All: I think we need to think a bit more about the names - I'm quite uncomfortable with forcing people who want the correct TrueEcliptic to now use a new class and then later have them change again. Is there really no better way?

As a possible suggestion: could we perhaps use the fact that obstime is a new parameter? If you give it, you get the correct frame, if you do not, you get a warning and get the "wrong" one? This would be quite easy to do by adding a __new__ method to TrueEcliptic, which returns a MeanEcliptic instance if obstime is not given. We could even ensure that the default is something senseless (like False) so that obstime=None works as expected (and will continue to work).

@mhvk

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

Oops, maybe that obstime trick only works for some?

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

All: I think we need to think a bit more about the names - I'm quite uncomfortable with forcing people who want the correct TrueEcliptic to now use a new class and then later have them change again. Is there really no better way?

An alternative is to treat the current situation as a bug, and say "look, we changed the semantics of *TrueEcliptic frames, but they were wrong anyway so now you will thank us - and if you want a *MeanEcliptic frame, there is a new class for it".

Not sure about the obstime trick, we can give it some thought but it was added to some frames in 3.0, and to some other frames now because, well, we forgot.

@mhvk

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

I'm actually quite happy with the "this was a bug, we now corrected it" approach, but @eteq may not be...

@adrn

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

@Juanlu001 Thanks for your hard work on this! I'd also be fine with taking the view that this was a bug...but @eteq is definitely more familiar with the ecliptic frames, so I would feel more comfortable with him taking another look at this. Maybe try emailing him directly?

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

@eteq Sorry for being a pain in the neck 😇 If I'm not mistaken, 3.2 feature freeze is in three weeks from now. Taking into account that the next version will (likely) be 4.0, I'd really love to get this in for 3.2, and having some days to do relaxed reviews (just in case we can avoid a last-minute sprint two days before the crucial date). If you agree to treat the present state as a bug, I will "fix" it in this PR - if not, I'm open to having a telecon so we can try to reach consensus.

@eteq

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Hmm. I see your points @mhvk @Juanlu001 @adrn. My biggest worry here is that I know at least one person (who talked to me but is not active on github) "worked around" this in 3.1, so their code will suddenly be broken again. But you're right it is a bug so it's legit to just treat it that way.

How about this plan: we do as @Juanlu001 says in #8394 (comment) and have this PR just fix it. If I (or anyone else) scrapes together the time, we could potentially have a follow-on PR to move it to my deprecation period proposal, or some other way to try to catch the concern I have above. But I agree it's better to get a fix in for 3.2 vs nothing, so this way even if there's not time/energy for that we get the fix it.

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Thanks for chiming in @eteq, sounds like a plan! I'll get back to this ASAP

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Can you add a what's new entry about this? No need to add any detail, just a small placeholder to remind us to add more words in time for release.

Sure!

There's still a warning in the base ecliptic docstring about it being "untested". I think that can go if we're also removing it from the narrative docs?

Oops, I thought I had removed them all. Doing!

from .ecliptic import (GeocentricMeanEcliptic, BarycentricMeanEcliptic,
HeliocentricMeanEcliptic,
GeocentricTrueEcliptic, BarycentricTrueEcliptic,
HeliocentricTrueEcliptic,

This comment has been minimized.

Copy link
@adrn

adrn Apr 19, 2019

Member

I think you need to add all of the new frames here if you want them to appear in the API documentation - right now, HeliocentricEclipticIAU76 and CustomBarycentricEcliptic are missing from the docs in this PR

bary_sun_pos = get_body_barycentric('sun', from_coo.obstime)

newrepr = intermed_repr + bary_sun_pos
return to_frame.realize_frame(newrepr)

This comment has been minimized.

Copy link
@adrn

adrn Apr 19, 2019

Member

Ah, sounds like we should add an option to let us decide the order of operations...

@adrn
Copy link
Member

left a comment

Overall looks ok to me (but I did not review the transformation code in any detail). One minor issue is that the new classes don't appear in the documentation - see inline comment.

@eteq

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

I took it on myself to write a quick test to boost coverage to hit all the new frames (see f2e1f91) and in the process I had to solve the issue @adrn pointed out in the process (beba865).

Sounds like the AffineTransform thing is a later PR then, so I think this is probably good if my two commits make the tests pass?

@eteq

eteq approved these changes Apr 19, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Thanks @eteq, you were faster than me :) I was looking into adding some numerical tests cases but deadlines are deadlines.

@adrn

adrn approved these changes Apr 19, 2019

@adrn
Copy link
Member

left a comment

Just noticed that this still needs a What's New entry! (or do you want to do that in a later PR?)

@adrn

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

Ah, on second thought, unless I misunderstood your comment, I think you can actually replace the FunctionTransform's using the current AffineTransform class. It's true that the AffineTransform assumes you want (1) A x + b, whereas it sounds like you want (2) A(x + b). But you can return something in the form of (1) by applying your matrix to the offset, so if you return from the function A, Ab instead of A, b, I think this should do what you want? (If I understood correctly, this is similar to what I had implemented in galactocentric).

eteq added some commits Apr 19, 2019

@eteq

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

OK, I added the what's new entry and fixed an ensuing merge conflict. I think this is good to go if @adrn agrees and the tests pass!

@eteq

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

I'm thinking in the current state it's best to merge this as it stands without the AffinteTransform updates. Since that wouldn't really be a feature change we could squeak it in later before 3.2, or do it in 4.0 and it's ok either way. Just don't want it to block the main deal here.

@adrn

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

OK, that's fine with me, but please make an issue!

@adrn

adrn approved these changes Apr 19, 2019

@bsipocz bsipocz merged commit bbc1219 into astropy:master Apr 19, 2019

15 checks passed

astropy-bot:changelog Changelog entry consistent with milestone
astropy-bot:milestone This pull request has a milestone set.
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl202 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl212 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl222 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl300 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpldev Your tests passed on CircleCI!
Details
codecov/patch 87.64% of diff hit (target 86.91%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +0.72% compared to 3bdd0ec
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
giles Click details to preview the documentation build
Details
@bsipocz

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

All green so I'm merging. Thanks @Juanlu001!

@Juanlu001 Juanlu001 deleted the Juanlu001:more-ecliptic-frames branch Apr 19, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Thanks @bsipocz! Adding one more on top of this one which I was about to commit :) #8606

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.