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

Use QTable for IERS throughout #9226

Merged
merged 3 commits into from Sep 14, 2019
Merged

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Sep 13, 2019

#9204 made me look at the IERS code again and I realized it could use some cleanup. The first commit ensures we just use QTable rather than go through Table and then fill the masked columns. This makes merging/selecting between A and B quite a bit cleaner. The second commit removes the IERS.read method (which was just assigned to IERS_B.read) so that super class work; it also prevents warnings about dropped masks.

A small additional change is to ensure that in Time the default IERS table is always that from IERS_Auto.

cc @aarchiba

@mhvk mhvk added this to the v4.0 milestone Sep 13, 2019
@mhvk mhvk requested a review from taldcroft September 13, 2019 21:01
@astropy-bot astropy-bot bot added the time label Sep 13, 2019
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Get the table index for the first row that has predictive values
# PolPMFlag_A IERS (I) or Prediction (P) flag for
# Bull. A polar motion values
# UT1Flag_A IERS (I) or Prediction (P) flag for
# Bull. A UT1-UTC values
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done.

@mhvk
Copy link
Contributor Author

mhvk commented Sep 14, 2019

@taldcroft - I thought about deprecating the bare IERS class - really, it should have been an IERSBase in the first place - but ended up thinking it might be better done as part of #9227 (triggered by the issues raised by @aarchiba).

p.s. And, yes, I tested locally with networking enabled and with a local finals file (a test that never gets done in CI - another thing I would now have done differently!)

@bsipocz
Copy link
Member

bsipocz commented Sep 14, 2019

p.s. And, yes, I tested locally with networking enabled and with a local finals file (a test that never gets done in CI - another thing I would now have done differently!)

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.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhvk - looks good to merge to me if you think it's done.

@mhvk
Copy link
Contributor Author

mhvk commented Sep 14, 2019

@bsipocz - see #9232.

Merging this one, thanks for the review, @taldcroft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants