Repository navigation
Use QTable for IERS throughout - #9226
Conversation
aa60509 to
776f677
Compare
taldcroft
left a comment
taldcroft
left a comment
There was a problem hiding this comment.
This looks basically fine. In your local testing have you ensured that networking is enabled (no tests are skipped)?
Should we deprecate the bare IERS class now? I expect that if you were writing this whole thing today that it would look very different.
| is_predictive = (table['UT1Flag_A'] == 'P') | (table['PolPMFlag_A'] == 'P') | ||
| table.meta['predictive_index'] = np.min(np.flatnonzero(is_predictive)) | ||
| table.meta['predictive_mjd'] = table['MJD'][table.meta['predictive_index']] | ||
| table.meta['predictive_index'] = np.searchsorted(is_predictive, True) |
There was a problem hiding this comment.
This is a creative / funky use of searchsorted. Is this guaranteed to always work or does it just happen to work for the current implementation. It depends on a numerical ordering of True and False.
There was a problem hiding this comment.
It is a lot faster as it doesn't have to allocate an array of indices. And I think False == 0 and True == 1 is sufficiently baked into python...
In fact, one can do the searchsorted on the flag, which changes the whole thing from ~250 us to 3.5 us...
%timeit is_predictive = (table['UT1Flag_A'] == 'P') | (table['PolPMFlag_A'] == 'P'); np.min(np.flatnonzero(is_predictive))
# 1000 loops, best of 5: 238 µs per loop
%timeit min(np.searchsorted(table['UT1Flag_A'], 'P'), np.searchsorted(table['PolPMFlag_A'], 'P'))
# 100000 loops, best of 5: 3.49 µs per loop
| @@ -29,11 +29,11 @@ class TestBasic(): | |||
| """Basic tests that IERS_B returns correct values""" | |||
|
|
|||
| def test_simple(self): | |||
There was a problem hiding this comment.
Should this test just use both IERS and IERS_B as a test parameter? That would remove the need for the new test_iers_b_for_iers.
|
@taldcroft - I thought about deprecating the bare p.s. And, yes, I tested locally with networking enabled and with a local |
And not their base, IERS. This allows proper super() class in the read classmethods.
776f677 to
337a7ff
Compare
do we have an issue opened for it? maybe at some point someone has the time to come up for a CI solution for this corner case. |
|
Merging this one, thanks for the review, @taldcroft. |
#9204 made me look at the IERS code again and I realized it could use some cleanup. The first commit ensures we just use
QTablerather than go throughTableand then fill the masked columns. This makes merging/selecting between A and B quite a bit cleaner. The second commit removes theIERS.readmethod (which was just assigned toIERS_B.read) so thatsuperclass work; it also prevents warnings about dropped masks.A small additional change is to ensure that in
Timethe default IERS table is always that fromIERS_Auto.cc @aarchiba