Repository navigation
Use EarthLocation instead of GCRS position for TETE observer. - #11061
Conversation
mhvk
added
Bug
coordinates
Affects-dev
| @frame_transform_graph.transform(FunctionTransformWithFiniteDifference, GCRS, TETE) | ||
| def gcrs_to_tete(gcrs_coo, tete_frame): | ||
| # first apply GCRS self transform to correct time and observatory position/velocity | ||
| obsgeoloc, obsgeovel = tete_frame.location.get_gcrs_posvel(tete_frame.obstime) |
There was a problem hiding this comment.
This is definitely not the way to go here. You are calculating the BPN matrix twice. This should be done in the same manner as gcrs_to_cirs().
There was a problem hiding this comment.
This is why I liked @StuartLittlefair's EarthLocation.get_posvel() method.
There was a problem hiding this comment.
OK, that makes sense. I'll adjust. There is also a real failure in comparison with horizons, so I may be missing more than this...
There was a problem hiding this comment.
Just to be sure, is there still a benefit from having the GCRS<->TETE conversion explicitly defined?
There was a problem hiding this comment.
(On get_posvel(): only useful if we stick with obsgeoloc, obsgeovel for GCRS...)
There was a problem hiding this comment.
On obsgeopos/vel, I think it is mostly a question of whether it should instead be a GCRS instance that holds the position/velocity. Anyway, that is for #10996!
There was a problem hiding this comment.
p.s. Note that what I'm doing in the new commit is not really quite equivalent to get_gcrs_posvel(rbpn=None) -- am I still missing the main speed-up?
There was a problem hiding this comment.
Well, it basically is. It's just that it requires another function whereas all of this could be incorporated into a single EarthLocation.get_gcrs_posvel(rpbn=None) method. Anyway, this looks good for now.
There was a problem hiding this comment.
The main way to speed this up would be to chain matrices together; since we know this is a transform at the same obstime we are just doing rotations to get from ITRS->GCRS so you can avoid some of the flexibility of transform_to.
You could pass a list - in the correct order - of already calculated matrices to transform ITRS into GCRS like so:
matrix_chain = [matrix_transpose(tete_to_itrs_mat(frame.obstime)), rbpn]
loc_gcrs = get_location_gcrs(frame, matrix_chain)There was a problem hiding this comment.
In fact, that could just be
itrs_to_gcrs = matrix_transpose(tete_to_itrs_mat(frame.obstime) @ rbpn)
loc_gcrs = get_location_gcrs(frame, itrs_to_gcrs))
with indeed a simpler get_location_gcrs. (I may have swapped things...)
StuartLittlefair
left a comment
StuartLittlefair
left a comment
There was a problem hiding this comment.
Looking good! Some minor nit-picks but the code is fine IMO.
|
Note to self: this will need a separate PR to backport to 4.2, since I now use stuff from #10994. |
|
Well, that could be avoided by including #10994 in the yet to be released 4.2. |
|
#10994 and this are really one package, after all. |
|
I'll merge this and first of all make a back-port of just this to 4.2, since it is super self-contained, not changing any behaviour, and with, I think, next to zero risk of introducing new problems. But I'm happy to make an alternative that includes #10994. |
Use EarthLocation instead of GCRS position for TETE observer.
Use EarthLocation instead of GCRS position for TETE observer.
Use EarthLocation instead of GCRS position for TETE observer.
|
Backport was tricky so is done separately in #11065 |
Use EarthLocation instead of GCRS position for TETE observer.
Merge pull request #11061 from mhvk/tete-location
3 participants
As discussed in #10994 and #10996, the TETE frame that was introduced for 4.2 should more logically use
EarthLocationto indicate the location of the observer, instead ofobsgeolocandobsgeovel. This PR makes that change.One important question is whether this can still be included in 4.2, even though it is very late. The main advantage is that in that case we do not have to deprecate
obsgeolocandobsgeovel. (The PR is only a bug fix in this regard.)Input from @astropy/core-release-maintainers much appreciated!!
cc @mkbrewer, @StuartLittlefair