Fix loading of IERS data files by aarchiba · Pull Request #9204 · astropy/astropy

aarchiba · 2019-09-06T01:04:13Z

This test suite has them marked as xfail, but there are some troubling anomalies in the handling of the IERS B data. Tests demonstrate, and currently xfail:

The IERS B data is not loaded into the IERS_Auto object if new IERS A data is not available, even though the IERS B file is present.
When new IERS A data is available, the columns dX_2000A_B and dY_2000A_B are not loaded in from the (potentially new) IERS B data that is available, unlike UT1_UTC_B, PM_X_B, and PM_Y_B.

Also, not exactly a bug, but it is good to have tests to let us know when this has occurred:

The IERS will occasionally change existing, old, IERS B entries when releasing a new IERS B file. This can lead to surprising non-reproducibility, and it argues for ensuring that the version of IERS B distributed with astropy is recent (in hopes that they do not often change the old entries).

taldcroft · 2019-09-09T17:47:49Z

@aarchiba - thanks for this! I haven't looked at this in detail yet but it is on my to-do list unless @mhvk gets there first.

aarchiba · 2019-09-12T16:27:30Z

I believe that the PR now fixes both bugs (and has tests). It also tests for changes in IERS B between astropy's built-in IERS B and the published one on the places where they overlap. There remains an awkwardness to do with IERS_Auto and IERS B, specifically that IERS_Auto never updates the IERS B data.

mhvk

Thanks very much for this! Some initial comments. Note that I still haven't quite wrapped my head around the various things we actually want, and perhaps we should try to enumerate that. Is it roughly the following?

IERS_Auto should make "best estimate from A and B" for all five columns, not just the three UT1_UTC, PM_X and PM_Y (included here).
IERS_Auto without on-line connection should make a look-alike file from the IERS_B file (included here).
It should be possible to have IERS_B be up to date (not included here).
Any update on IERS_B should propagate to IERS_Auto (not included here).

I think (1) is uncontroversial. (2) is less obvious to me - if we do it, I think all columns should be populated, also *_A ones (or at least the column with the flag). I'd like to understand, though, whether it is useful to have those extra columns, or perhaps rather, whether it still would be useful if we did have (3) and possibly (4).

mhvk · 2019-09-12T17:15:11Z

        in the downloaded IERS-A table.
        """
-        iers_b = IERS_B.open()
+        iers_b = IERS_B.open(IERS_B_FILE)


I think I'd prefer to change the docstring and use whatever IERS_B is available (possibly newer than bundled).

It turns out this makes testing extremely frustrating. The astropy cache provides a kind of shared global state that makes systematic testing very difficult - I finally gave up and did this when the previous approach gave me unpredictable results. (Note that PR #9182 provides a solution to this particular problem.) I think the solution is actually a larger structural change.

I think with adequate documentation this could be made to work:

If the user wants a specific table version, use IERS_B.open("iers_b.table")

If the user wants the built-in version, use IERS_B.open(IERS_B_FILE)

If the user wants the cached version, falling back to IERS_B_FILE or raising an error for IERS_A, they can just do IERS_B.open()

If the user wants auto-updating, use IERS_Auto.open() (which starts with whatever is in cache).

This works for earth_rotation_table as well as direct use of the tables.

It's still different from now, where using no-argument open gets you whatever is stored in the class-wide cls.iers_table. But I can make that change fairly easily.

Agreed that from a user perspective, with the new earth_rotation_table, it is possible to select whatever one wants. Here, it is about the behaviour of IERS_Auto - where does it gets its IERS-B values. Choices are:

Use IERS-A IERS-B values (but those are not quite OK);

Use bundled IERS-B file (but can be out of date; would happen with your change);

Use whatever the current default is - bundled until something specific downloaded prior to IERS_Auto() (current behaviour; note: currently, any IERS_B_URL downloaded in a previous session is ignored);

Use most current already downloaded version (i.e., as current, but do not ignore previously downloaded IERS-B);

Ensure IERS-B is kept up to date in the same way IERS-A is.

For this PR, my tendency would be to document the present state...

Is this now resolved?

mhvk · 2019-09-12T17:17:28Z

+
+
+@pytest.mark.parametrize("keyword,atol", test_columns)
+@pytest.mark.xfail(


Since we turned expected failures that fail to fail into failures (parse that!), this seems a bit risky: don't really want CI to start failing when we update the bundled IERS_B and a downloaded IERS_B actually agrees with it! I guess we need some kind of "allowed fail".

Well, I don't know - maybe it's good if something lets us know when we've changed the bug profile of the code base? The sequence will be: someone drops in a new IERS B table, the test suite grinds to a halt with XPASS, they say "huh", they remove the xfail notification, hopefully leaving an explanation, problem done.

I guess the question then becomes why we are testing what IERS is doing? The tests should test that our code makes sense. In principle, we could run it with snippets of files for which the outcome is guaranteed to be correct (that's done with some IERS_A snippets in iers/tests/data)

This is a different category of test, I guess - not "does our code do something wrong?" but "has upstream done anything we need to know about?". Maybe not appropriate as part of the test suite, exactly, but I think maybe we do need to know about it?

Not being familiar with IERS updates and such, I just want to point out that we have set pytest to fail on XPASS with:

astropy/setup.cfg

Line 105 in 4bf2d41

xfail_strict = true

As a result, if XPASS starts to happen, CI will fail even for unrelated push events (e.g., someone else's PR). Is this what you are going for? If not, I would advise to rethink this test setup.

mhvk · 2019-09-15T18:53:38Z

@aarchiba - I factored out the addition of dX..., dY... to #9237, so that we can at least get that uncontroversial piece in (rebased it of a commit of yours, so credited appropriately).

For the default for off-line of just using IERS B (current) versus making it a mock-up of the IERS A/B combination, I'm less sure what the best path is. We may be better off lopping off items in #9227.

bsipocz · 2019-09-17T16:03:05Z

Now that #9237 is merged, this needs a rebase.

IERS_Auto merges in columns from the IERS B table. But as it stood, the version of IERS B that was used was difficult to control. Now it always uses the version built in to astropy.

pllim

@mhvk and @taldcroft , do you have time to do a final review before feature freeze? Thanks!

pllim · 2019-10-24T17:36:16Z

        in the downloaded IERS-A table.
        """
-        iers_b = IERS_B.open()
+        iers_b = IERS_B.open(IERS_B_FILE)


Is this now resolved?

pllim · 2019-10-24T17:39:29Z

+
+
+@pytest.mark.parametrize("keyword,atol", test_columns)
+@pytest.mark.xfail(


Not being familiar with IERS updates and such, I just want to point out that we have set pytest to fail on XPASS with:

astropy/setup.cfg

Line 105 in 4bf2d41

xfail_strict = true

As a result, if XPASS starts to happen, CI will fail even for unrelated push events (e.g., someone else's PR). Is this what you are going for? If not, I would advise to rethink this test setup.

pllim · 2019-10-24T17:40:39Z

Also needs a change log. Thanks for your patience, @aarchiba !

mhvk · 2019-10-24T18:44:22Z

@aarchiba - your PR directly led to #9244, which I think solves most of the probllems, hopefully more elegantly. Given that, is this PR still useful? My sense would be to eventually deprecate the caching on classes, and move to having IERS_A_Auto() and IERS_B_Auto() classes, with IERS_Auto providing the former if possible, and the latter if not.

aarchiba · 2019-10-24T19:46:07Z

Yes I need to take a look at this and see what if any of it is relevant in light of the ScienceState stuff - maybe just the tests, maybe nothing, maybe there is some useful code here.

aarchiba · 2019-10-24T21:04:27Z

Some of the tests are probably still useful to make sure that table merging does something sensible, and we might still need the from_iers_b to cover when IERS A is not available. It's a little bit hard to judge as is - maybe I should rebase it on top of #9244 ? Does that make sense? I mean I think this only makes sense as an extension of the ScienceState work.

I think, too, it's fair to say that this PR is not needed to introduce any new features or make significant API changes, just to (I hope) enhance testing and reveal and fix bugs. So perhaps it can wait for #9244 ?

pllim · 2019-10-28T21:04:53Z

#9244 is merged. Is this still on track for 4.0?

bsipocz · 2019-11-14T21:45:26Z

What's the status here?

bsipocz · 2019-11-19T00:13:18Z

@aarchiba @mhvk @pllim - What are the plans with this in respect of 4.0?

pllim · 2019-11-19T00:52:08Z

Let's punt this to 4.0.1.

pllim · 2019-12-12T22:10:47Z

This now has conflict.

astropy-bot · 2020-02-20T08:01:04Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

astropy-bot · 2020-03-23T08:02:17Z

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

astropy-bot Bot added the utils label Sep 6, 2019

This was referenced Sep 6, 2019

IERS B not always loaded into IERS_Auto #9205

Closed

Incomplete loading of IERS B data into IERS_Auto #9206

Closed

pllim added the Experimental label Sep 6, 2019

aarchiba changed the title ~~Test loading of IERS data files to reveal several bugs~~ Fix loading of IERS data files Sep 9, 2019

This comment has been minimized.

Sign in to view

taldcroft requested review from mhvk and taldcroft September 9, 2019 17:48

mhvk reviewed Sep 12, 2019

View reviewed changes

This was referenced Sep 13, 2019

Use QTable for IERS throughout #9226

Merged

Possible improvements to how IERS is handled #9227

Closed

Ensure that nutation columns are also copied from IERS B to IERS_Auto #9237

Merged

aarchiba added 6 commits September 17, 2019 20:50

Test loading of IERS data files

aa6dced

Insert bug numbers

fc350a0

Fixes astropy#5906

1f5b12f

Fixes astropy#9205

bb02bb5

Fix astropy#9206 by making IERS B use deterministic

81752a5

IERS_Auto merges in columns from the IERS B table. But as it stood, the version of IERS B that was used was difficult to control. Now it always uses the version built in to astropy.

Clarify that IERS_Auto does two things

61eb74b

aarchiba force-pushed the iers_bug branch from 4fcf1c9 to 61eb74b Compare September 17, 2019 19:09

aarchiba added 2 commits September 17, 2019 21:29

Move IERS_B conversion function into IERS_Auto

5cb676f

Use built-in B for determinism

e4ba64e

bsipocz added this to the v4.0 milestone Sep 17, 2019

aarchiba added 2 commits September 18, 2019 01:29

Mark test as needing remote data.

56c0fe6

Fix silly bug that triggered downloading

181c5da

pllim removed the Experimental label Oct 24, 2019

pllim reviewed Oct 24, 2019

View reviewed changes

pllim added Bug Affects-release labels Oct 24, 2019

pllim modified the milestones: v4.0, v4.0.1 Nov 19, 2019

taldcroft removed their request for review December 12, 2019 22:15

bsipocz modified the milestones: v4.0.1, v4.0.2 Mar 21, 2020

astropy-bot Bot added the closed-by-bot Closed by stale bot label Mar 23, 2020

astropy-bot Bot closed this Mar 23, 2020



		@pytest.mark.parametrize("keyword,atol", test_columns)
		@pytest.mark.xfail(

Conversation

aarchiba commented Sep 6, 2019

Uh oh!

This comment has been minimized.

taldcroft commented Sep 9, 2019

Uh oh!

aarchiba commented Sep 12, 2019

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Sep 15, 2019

Uh oh!

bsipocz commented Sep 17, 2019

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 24, 2019

Uh oh!

mhvk commented Oct 24, 2019

Uh oh!

aarchiba commented Oct 24, 2019

Uh oh!

aarchiba commented Oct 24, 2019

Uh oh!

pllim commented Oct 28, 2019

Uh oh!

bsipocz commented Nov 14, 2019

Uh oh!

bsipocz commented Nov 19, 2019

Uh oh!

pllim commented Nov 19, 2019

Uh oh!

pllim commented Dec 12, 2019

Uh oh!

astropy-bot Bot commented Feb 20, 2020

Uh oh!

astropy-bot Bot commented Mar 23, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants