Skip to content

Fix remote test failures related to IERS update - #10387

Merged
astrofrog merged 4 commits into
astropy:masterfrom
astrofrog:fix-remote
May 24, 2020
Merged

Fix remote test failures related to IERS update#10387
astrofrog merged 4 commits into
astropy:masterfrom
astrofrog:fix-remote

Conversation

@astrofrog

@astrofrog astrofrog commented May 22, 2020

Copy link
Copy Markdown
Member

This fixes two of the failures mentioned by @pllim in #10386. However, I'm not sure how to deal with the following:

________________ test_IERS_B_parameters_loading_into_IERS_Auto _________________
    @pytest.mark.remote_data
    def test_IERS_B_parameters_loading_into_IERS_Auto():
        A = iers.IERS_Auto.open()
        B = iers.IERS_B.open()
    
        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])
    
        assert np.all(np.diff(i_B) == 1), "Valid region not contiguous"
        assert np.all(A["MJD"][ok_A] == B["MJD"][i_B])
        # Check that values are copied correctly.  Since units are not
        # necessarily the same, we use allclose with very strict tolerance.
        for name in ("UT1_UTC", "PM_x", "PM_y", "dX_2000A", "dY_2000A"):
            assert_quantity_allclose(
                A[name][ok_A], B[name][i_B], rtol=1e-15,
                err_msg=("Bug #9206 IERS B parameter {} not copied over "
>                        "correctly to IERS Auto".format(name)))
../../.tox/py37-test-devdeps/lib/python3.7/site-packages/astropy/utils/iers/tests/test_iers.py:330: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
actual = <Quantity [ 0.8078584,  0.8051525,  0.8024015, ..., -0.2382365, -0.2385745,
           -0.2390142] s>
desired = <Quantity [ 0.8078584,  0.8051525,  0.8024015, ..., -0.2382558, -0.2385857,
           -0.239021 ] s>
rtol = 1e-15, atol = None
kwargs = {'err_msg': 'Bug #9206 IERS B parameter UT1_UTC not copied over correctly to IERS Auto'}
np = <module 'numpy' from '/home/travis/build/astropy/astropy/.tox/py37-test-devdeps/lib/python3.7/site-packages/numpy/__init__.py'>
_unquantify_allclose_arguments = <function _unquantify_allclose_arguments at 0x7f6b3dd97a60>
    def assert_quantity_allclose(actual, desired, rtol=1.e-7, atol=None,
                                 **kwargs):
        """
        Raise an assertion if two objects are not equal up to desired tolerance.
    
        This is a :class:`~astropy.units.Quantity`-aware version of
        :func:`numpy.testing.assert_allclose`.
        """
        import numpy as np
        from astropy.units.quantity import _unquantify_allclose_arguments
        np.testing.assert_allclose(*_unquantify_allclose_arguments(
>           actual, desired, rtol, atol), **kwargs)
E       AssertionError: 
E       Not equal to tolerance rtol=1e-15, atol=0
E       Bug #9206 IERS B parameter UT1_UTC not copied over correctly to IERS Auto
E       Mismatched elements: 18 / 17275 (0.104%)
E       Max absolute difference: 2.43e-05
E       Max relative difference: 0.00010208
E        x: array([ 0.807858,  0.805153,  0.802401, ..., -0.238236, -0.238574,
E              -0.239014])
E        y: array([ 0.807858,  0.805153,  0.802401, ..., -0.238256, -0.238586,
E              -0.239021])
../../.tox/py37-test-devdeps/lib/python3.7/site-packages/astropy/tests/helper.py:483: AssertionError
_____________________________ [doctest] frames.rst _____________________________

I re-ran the update script hence the small updates in this PR, but that didn't help - the failures still happen.

@astrofrog
astrofrog requested a review from mhvk May 22, 2020 18:07
@astrofrog astrofrog added this to the v4.1 milestone May 22, 2020
@astrofrog
astrofrog requested a review from eteq May 22, 2020 18:13
@eteq

eteq commented May 22, 2020

Copy link
Copy Markdown
Member

Hmm. I'll have to do some local tests, but I wonder if this is due to the recent changes in how the IERS files are delivered, such that they were out-of-sync at the time the update happened?

@pllim pllim added Affects-dev PRs and issues that do not impact an existing Astropy release and removed no-changelog-entry-needed labels May 22, 2020
@astrofrog

astrofrog commented May 22, 2020

Copy link
Copy Markdown
Member Author

I updated them a few days ago, and then again today, and in both cases the remote failures have happened - very strange!

@eteq

eteq commented May 22, 2020

Copy link
Copy Markdown
Member

Hmm. The mystery deepens: try as I might I cannot reproduce this locally - the test just keeps passing. I wonder if this is a location problem? I.e., @astrofrog (and travis) might be getting European servers but I'm getting a us one?

@eteq

eteq commented May 22, 2020

Copy link
Copy Markdown
Member

Oh wait, nevermind - it's even more insidious: apparently this test passes on py3.8 but not 3.7. I can reproduce now though so perhaps I can make comparisons between the two envs and work it out.

@astrofrog

astrofrog commented May 22, 2020

Copy link
Copy Markdown
Member Author

@mhvk @taldcroft - any ideas what could be going on here?

@astrofrog

astrofrog commented May 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

I've tracked the issue down to the following line:

astropy/astropy/utils/iers/iers.py

Line 803 in 4d754b4

mjd_b = table['MJD'][np.isfinite(table['UT1_UTC_B'])]

in _substitute_iers_b - if I remove the finite value filtering, everything works, so I think there is a mismatch between whether non-finite values are ignored or not in the test too

@eteq

eteq commented May 22, 2020

Copy link
Copy Markdown
Member

Hmm, so maybe it's just that past IERS B/A combinations didn't have empty UT1-UTC entries? I think is what gives non-finite answers there as it maps onto nans in the resulting table. So it's possible this was wrong all along but we just didn't realize it because there weren't problematic tables?

I'm still struggling to find any good matches by actually looking at the files directly. I guess that was the purpose of the substitution in the first place but it may be this is due for a more complicated clean-up in 4.2 if we can band-aid this for now...

(And to continue my more prosaic line of inquiry above: in a new py38 environment it's also now failing so it was probably a red herring due to something stuck in my initial environment)

@astrofrog

astrofrog commented May 22, 2020

Copy link
Copy Markdown
Member Author

I've pushed a commit that resolves the issue for me locally, though I'm not sure if it's the right fix. But conceptually, doing the filtering to me seems odd since later on it seems we don't pre-filter when assigning values to the table.

@mhvk - I think we really need your expertise here to determine whether this is the right fix thought! (assuming CI actually passes)

@astrofrog

astrofrog commented May 22, 2020

Copy link
Copy Markdown
Member Author

While this seems to fix the problematic test, other tests are now failing 😢

@pllim

pllim commented May 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

isfinite was added in #9226 .

Looks like now delta = iers_table.ut1_utc(self.t.jd1, self.t.jd2) has different result in TestIERS_Auto.test_auto_max_age_none.

@astrofrog

astrofrog commented May 22, 2020

Copy link
Copy Markdown
Member Author

If anyone wants to push commits here please feel free to!

@astrofrog astrofrog mentioned this pull request May 23, 2020
Closed
17 of 19 tasks
@mhvk

mhvk commented May 24, 2020

Copy link
Copy Markdown
Contributor

OK, I at least now understand the problem: IERS_A includes also IERS_B values, but those are not always identical to the actual IERS_B (since, surprisingly, those change). So, we wrote code to update with values from IERS_B; the np.isfinite is to restrict the copying to replacing those values it had in the first place. But this goes wrong in this case, since we now have a very new IERS_B which has values beyond the "iers_b" values included in IERS_A. And clearly the test did not take into account that this might happen.

I must admit that I'm not sure what the right behaviour really should be; one could argue for just replacing everything, but on the other hand the iers_b bundled in iers_a might be more up to date. My own sense would be that IERS_Auto should also download IERS_B and just make its own mix. But that's really for another PR, so I think here it makes most sense to simply adjust the test so that it actually tests what the current code is supposed to do. I'll push a fix with that. Then, we can open another issue to think through what the ultimate behaviour should be.

@mhvk mhvk mentioned this pull request May 24, 2020
Closed
1 of 7 tasks
@mhvk

mhvk commented May 24, 2020

Copy link
Copy Markdown
Contributor

And there was an issue more or less about this (but without the utils.iers label, now corrected; maybe iers should just move up...). See #9227.

mhvk
mhvk approved these changes May 24, 2020
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving, though since it is now partially my work, won't merge yet until others have had a look.

@astrofrog

astrofrog commented May 24, 2020

Copy link
Copy Markdown
Member Author

Thanks for fixing this! I agree this is the right approach for now (just making sure the test matches what's actually done). I'll go ahead and merge this since the remote test build is now passing.

@astrofrog
astrofrog merged commit cf079cc into astropy:master May 24, 2020
astrofrog added a commit that referenced this pull request May 24, 2020
@astrofrog
Merge pull request #10387 from astrofrog/fix-remote
d142180 
Fix remote test failures related to IERS update
astrofrog added a commit that referenced this pull request May 24, 2020
@astrofrog
Merge pull request #10387 from astrofrog/fix-remote
e0810af 
Fix remote test failures related to IERS update
@eteq

eteq commented May 25, 2020

Copy link
Copy Markdown
Member

Ohhh, that explains it @mhvk - it hadn't occurred to me that IERS B might be too new!

While investigating this (although not solving it without your input...) I also encountered a few other confusing things about IERS B but I guess #9227 is the place to discuss that

@astrofrog astrofrog modified the milestones: v4.1, v4.0.2 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk approved these changes
@eteq eteq Awaiting requested review from eteq

Assignees

No one assigned

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release coordinates utils.iers utils

Projects

None yet

Milestone

v4.0.2

Development

Successfully merging this pull request may close these issues.

4 participants

@astrofrog @eteq @pllim @mhvk