Add more ecliptic frames - #8394

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

@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.

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

@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.

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!

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?

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.

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.

TruerEcliptic? UltimateTruthEcliptic? TrulyEcliptic? DontUseTheOtherOneTrueEcliptic? SupremeTrueEcliptic?

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

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

@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?

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).

Oops, maybe that obstime trick only works for some?

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.

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

@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?

@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.

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.

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

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?

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

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).

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!

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.

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

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

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