Repository navigation
Ensure that nutation columns are also copied from IERS B to IERS_Auto - #9237
Conversation
|
Fixes #9206 |
| table['PM_X_B'][:n_iers_b] = iers_b['PM_x'] | ||
| table['PM_Y_B'][:n_iers_b] = iers_b['PM_y'] | ||
| table['dX_2000A_B'][:n_iers_b] = iers_b['dX_2000A'] | ||
| table['dY_2000A_B'][:n_iers_b] = iers_b['dY_2000A'] |
There was a problem hiding this comment.
Is unit conversion handled correctly here? I suppose the test verifies that the unit matches the number, but is it asking for trouble to have these values in a different unit in exceptional circumstances? How often do users do .value without a .to(units) first because they "know" the units the quantity is expressed in? I know the PINT source code contains a fair number of .value without .to()... would just adding a .to(u.marcsec) here reduce user error? (For some reason the A and B tables express these quantities in different units, correctly described in their READMEs.)
There was a problem hiding this comment.
These are QTable, so their columns are just Quantity, and they will do the unit conversion correctly. The reason that previously explicit .to(unit).value were there was that we had this dance between Table, whiose Column can do masked values well but not units, and QTable, for which it is vice versa.
In any case, the units of the actual table columns here will remain the same as they always were (those of IERS_A), so in that sense there is no risk. If we go the route you suggested of mimicking an IERS_A even when the input is a pure IERS_B, then we may want to reconsider.
taldcroft
left a comment
taldcroft
left a comment
There was a problem hiding this comment.
Just one little question about the test, otherwise good.
| ok_A = A["MJD"] <= B["MJD"][-1] | ||
| assert not np.all(ok_A), "IERS B covers all of IERS A: should not happen" | ||
|
|
||
| i_B = np.searchsorted(B["MJD"], A["MJD"][ok_A]) |
There was a problem hiding this comment.
This seems subject to off-by-one index errors for times where A["MJD"] is just slightly more (float error) than the corresponding B["MJD"]. Maybe it doesn't matter? I think in situations like this I have used nearest neighbor interpolation on an index array. However, I don't know if that can be easily done without scipy interpolate.
There was a problem hiding this comment.
The MJD are int, so I think it is OK.
There was a problem hiding this comment.
(well, they are float but represent int values)
This takes an uncontroversial piece from #9204 by @aarchiba - which is that
dX_2000AanddY_2000Ashould also be copied from IERS B toIERS_Auto- and rebased it on top of the refactoring in #9226.@aarchiba - I slightly rewrote your test (and moved it), so that it now tests that the
IERS_Autovalues that one would use are actually the same as those in theIERS_Bfile. I think this is the most important test, as it is what is actually used in astropy.