Fix IERS_Auto logic now that IERS-A is bundled #16187

ayshih · 2024-03-12T02:04:08Z

Description

#14819 moved the IERS files to a separate package (astropy-iers-data), and now IERS-A is essentially bundled with astropy. However, there persist some remnants of the old logic, where IERS-A is available only if the download were triggered. This results in the peculiar behavior that if automatic downloading of IERS-A files is disabled (iers.conf.auto_download = False) or fails, the bundled IERS-A file is ignored even though it is present (see #13227 (comment)).

This PR fixes the logic so that the bundled IERS-A file is used if iers.conf.auto_download = False, with corresponding fixes in documentation and tests.

This PR does not attempt to fix the logic so that a previously downloaded IERS-A file is used if iers.conf.auto_download is subsequently set to False. I didn't want to wrap my head around whether the downloaded file should have priority over the bundled file, given that astropy-iers-data could also be upgraded.

Fixes #13227: With astropy 6.0, the response to the report would be to upgrade astropy-iers-data to obtain a recent IERS-A table. This PR then fixes the residual bug where setting iers.conf.auto_download = False would ignore that bundled IERS-A table.

Fixes #16128: Because the bundled IERS-A is now no longer ignored by the default IERS_Auto when downloading is blocked, it's now typically impossible to trigger the "degraded accuracy" error/warning unless the user expressly forces the use of the IERS-B data alone (e.g., through the earth_orientation_table ScienceState).

Other related issues:

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-03-12T02:04:36Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions · 2024-03-12T02:04:45Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

neutrinoceros · 2024-03-12T07:39:43Z

Just a heads-up that this patch seems to touch some of the same lines than #16070, so merge conflicts might ensue. That said, my own PR is currently stuck so I don't have a problem with this one going in first, mind you. I'm also happy to serve as a reviewer if needed !

ayshih · 2024-03-12T13:31:36Z

I think #16070 will be sufficiently orthogonal – improving how IERS-A and IERS-B are combined versus merely ensuring that the combination even happens – that any merge conflicts should be trivial to resolve.

pllim

Thanks! Does the content of https://docs.astropy.org/en/stable/utils/data.html also need updating?

ayshih · 2024-03-12T18:14:46Z

Ah, indeed, that page should be edited too

mhvk

Thanks so much! I have one question on the implementation, whether it might not be even simpler to just preserve whatever has been downloaded (which in itself would seem more logical). What do you think? I am happy to go with what you have as well, as it resolves the biggest issues.

Beyond that, only a nitpick on the changelog entry...

mhvk · 2024-03-12T19:49:25Z

docs/changes/utils/16187.api.rst

@@ -0,0 +1 @@
+The return type of ``IERS_Auto.open()`` is no longer ``IERS_B`` when automatic updating of the IERS-A file is disabled or the downloading of the new file fails.


I think we should add what it is now! Maybe

IERS_Auto.open() now defaults to the combination of IERS_A and B data bundled in the astropy-iers-data dependency rather than to IERS_B only when automatic ...

Overhauled text

mhvk · 2024-03-12T19:57:34Z

astropy/utils/iers/iers.py

@@ -786,7 +790,7 @@ def open(cls):

        """
        if not conf.auto_download:


I guess the alternative path, which would preserve anything downloaded, would simply be to remove this stanza and in the else clause of the for loop warn only if conf.auto_download is set.

Am I missing something?

Discussion concluded we should leave this for follow-up

ayshih · 2024-03-13T15:43:29Z

I have one question on the implementation, whether it might not be even simpler to just preserve whatever has been downloaded (which in itself would seem more logical). What do you think?

Yes, that could be done, but the edge case that worries me is a user who does not want automatic downloading but at some point in the past happened to download IERS-A. Once that file is in the cache, that stale IERS-A file would continue to be used (by default) despite any subsequent updates of astropy-iers-data.

Of course, the natural solution would be to inspect both the IERS-A file from astropy-iers-data and any downloaded IERS-A file in the cache, and use whichever one has more recent non-predicted values, since that presumably means it is the more recent file. What do you think?

mhvk · 2024-03-13T17:41:09Z

@ayshih - yes, good point. So, I guess to get this to work one would have to also change _refresh_table_as_needed and in the download failure path check whether IERS_A_FILE is more up to date then the current one: effectively, for the no download case, one could do new_table = cls.read() there and fall through to the part that updates self as needed.

p.s. We can obviously move this to a new issue/PR!

ayshih · 2024-03-14T18:30:25Z

Let's get this PR in to solve the easy-to-encounter bug, and defer the improved handling of two IERS-A files to a future PR. I've updated the documentation more, including https://docs.astropy.org/en/stable/utils/data.html

mhvk

Looks good! (modulo the failure of course!)

astropy/utils/iers/iers.py

mhvk · 2024-03-14T19:52:32Z

docs/utils/data.rst

@@ -265,25 +263,19 @@ systems. The parallel access to the home directory can also trigger concurrency
 problems in the Astropy data cache, though we have tried to minimize these. We
 therefore recommend the following guidelines:

- * Do one of the following:
+ * Set ``astropy.utils.iers.conf.auto_download = False`` in your Astropy config


I like this change to have just one recommended way!

docs/utils/data.rst

docs/changes/utils/16187.api.rst

mhvk

@ayshih - I have a few more nitpicky comments on docstrings, to join those of @eerovaher. Will merge once those are done.

Thanks!

astropy/coordinates/tests/test_iau_fullstack.py

astropy/utils/iers/iers.py

mhvk · 2024-03-20T16:40:27Z

astropy/utils/iers/iers.py

@@ -786,7 +790,7 @@ def open(cls):

        """
        if not conf.auto_download:


Discussion concluded we should leave this for follow-up

neutrinoceros

Thank you so much for doing this. Not only is it fixing a subtle bug that has already confused several users, it's also making the underlying logic easier to reason about ! I only have a couple small suggestions and questions, but overall LGTM

neutrinoceros · 2024-03-29T14:37:36Z

astropy/coordinates/tests/test_iau_fullstack.py

    if PYTEST_LT_8_0:
-        ctx1 = ctx2 = nullcontext()
+        ctx1 = nullcontext()


nit: we might want to rename this variable ctx now. More importantly: thanks, seeing this test being simplified back is a nice surprise !

neutrinoceros · 2024-03-29T14:41:43Z

astropy/utils/iers/iers.py

-        "full IERS file with predictions was already downloaded and cached). "
+        "Enable auto-downloading of the latest IERS-A data.  If set to False "
+        "then the bundled IERS-A file will be used by default (even if a "
+        "newer version of the IERS-A file was already downloaded and cached). "


minor, but to me, "already" may suggest that this could have happened while running the current process.

Suggested change

"newer version of the IERS-A file was already downloaded and cached). "

"newer version of the IERS-A file was previously downloaded and cached). "

neutrinoceros · 2024-03-29T14:43:00Z

astropy/utils/iers/iers.py

-    The IERS A file is not part of astropy.  It can be downloaded from
-    ``iers.IERS_A_URL`` or ``iers.IERS_A_URL_MIRROR``. See ``iers.__doc__``
-    for instructions on use in ``Time``, etc.
+    If the package IERS A file (```iers.IERS_A_FILE``) is out of date, a new


Suggested change

If the package IERS A file (```iers.IERS_A_FILE``) is out of date, a new

If the package IERS A file (``iers.IERS_A_FILE``) is out of date, a new

neutrinoceros · 2024-03-29T14:50:11Z

astropy/utils/iers/tests/test_iers.py

+        # auto_download = False is tested in test_IERS_B_parameters_loading_into_IERS_Auto()
+
+    def teardown_class(self):
+        iers.conf.auto_download = False


Seems to me that this could potentially create test pollution if the default value was ever changed (for instance if we were to deprecated this parameter), or worse, it could hide pollution from other tests if they change this config value but forget to clean up after themselves.
While unlikely, these situation would be very painful to debug so I suggest to go with the cleaner approach from the get go, resetting the value to whatever it was before setup_class instead of hardcoding the current default value here.

neutrinoceros · 2024-03-29T14:52:44Z

astropy/utils/iers/tests/test_iers.py

+    # Double-check that auto downloading is off
+    assert not iers.conf.auto_download


Instead of a guard clause, we could use a pytest fixture, or a context manager here. Any reason not to ?

neutrinoceros · 2024-03-29T14:57:03Z

docs/utils/data.rst

- * Do one of the following:
+ * Set ``astropy.utils.iers.conf.auto_download = False`` in your ``astropy`` config
+   file (see :ref:`astropy_config`) or in your code so that ``astropy`` will not
+   automatically attempt to download a newer version of the IERS-A table than


Maybe I'm missing something and it's actually still possible to trigger the download manually ?Otherwise, I think this adverb could be dropped.

Suggested change

automatically attempt to download a newer version of the IERS-A table than

attempt to download a newer version of the IERS-A table than

neutrinoceros · 2024-03-29T15:04:15Z

astropy/utils/iers/tests/test_iers.py

@@ -495,7 +476,7 @@ def test_iers_download_error_handling(tmp_path):
                    "malformed IERS table from https://google.com"
                )
                assert str(record[2].message).startswith(
-                    "unable to download valid IERS file, using local IERS-B"
+                    "unable to download valid IERS file, using local IERS-A"


is this right ?

Suggested change

"unable to download valid IERS file, using local IERS-A"

"unable to download valid IERS file, using bundled IERS-A"

github-actions bot added Docs utils.iers labels Mar 12, 2024

ayshih force-pushed the iers_a branch from 62aea2a to 7c17771 Compare March 12, 2024 03:12

Fix IERS_Auto logic now that IERS-A is bundled

7ea06a4

ayshih force-pushed the iers_a branch from 7c17771 to 7ea06a4 Compare March 12, 2024 03:23

ayshih marked this pull request as ready for review March 12, 2024 04:31

pllim added this to the v6.1.0 milestone Mar 12, 2024

pllim added Bug API change PRs and issues that change an existing API, possibly requiring a deprecation period labels Mar 12, 2024

pllim reviewed Mar 12, 2024

View reviewed changes

pllim requested review from taldcroft, astrofrog and mhvk March 12, 2024 15:25

mhvk reviewed Mar 12, 2024

View reviewed changes

mhvk reviewed Mar 14, 2024

View reviewed changes

Documentation updates

9f1bd1c

ayshih force-pushed the iers_a branch from e298744 to 9f1bd1c Compare March 14, 2024 22:31

eerovaher reviewed Mar 18, 2024

View reviewed changes

docs/utils/data.rst Outdated Show resolved Hide resolved

eerovaher reviewed Mar 18, 2024

View reviewed changes

docs/changes/utils/16187.api.rst Outdated Show resolved Hide resolved

mhvk reviewed Mar 20, 2024

View reviewed changes

Addressed comments

9435353

ayshih mentioned this pull request Mar 21, 2024

Add GONG H-Alpha map source sunpy/sunpy#7451

Merged

5 tasks

neutrinoceros reviewed Mar 29, 2024

View reviewed changes

neutrinoceros mentioned this pull request Apr 3, 2024

ENH: include exclusive values from IERS_B in IERS_Auto (⏰ wait for #16187) #16070

Draft

1 task

astrofrog modified the milestones: v6.1.0, v7.0.0 Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix IERS_Auto logic now that IERS-A is bundled #16187

Fix IERS_Auto logic now that IERS-A is bundled #16187

ayshih commented Mar 12, 2024 •

edited

github-actions bot commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

neutrinoceros commented Mar 12, 2024

ayshih commented Mar 12, 2024

pllim left a comment

ayshih commented Mar 12, 2024

mhvk left a comment

mhvk Mar 12, 2024

ayshih Mar 14, 2024

mhvk Mar 12, 2024

mhvk Mar 20, 2024

ayshih commented Mar 13, 2024

mhvk commented Mar 13, 2024

ayshih commented Mar 14, 2024

mhvk left a comment

mhvk Mar 14, 2024

mhvk left a comment

mhvk Mar 20, 2024

neutrinoceros left a comment

neutrinoceros Mar 29, 2024

neutrinoceros Mar 29, 2024

neutrinoceros Mar 29, 2024

neutrinoceros Mar 29, 2024

neutrinoceros Mar 29, 2024

neutrinoceros Mar 29, 2024

neutrinoceros Mar 29, 2024

		@@ -0,0 +1 @@
		The return type of ``IERS_Auto.open()`` is no longer ``IERS_B`` when automatic updating of the IERS-A file is disabled or the downloading of the new file fails.

		@@ -786,7 +790,7 @@ def open(cls):

		"""
		if not conf.auto_download:

	"newer version of the IERS-A file was already downloaded and cached). "
	"newer version of the IERS-A file was previously downloaded and cached). "

	If the package IERS A file (```iers.IERS_A_FILE``) is out of date, a new
	If the package IERS A file (``iers.IERS_A_FILE``) is out of date, a new

		# Double-check that auto downloading is off
		assert not iers.conf.auto_download

	automatically attempt to download a newer version of the IERS-A table than
	attempt to download a newer version of the IERS-A table than

	"unable to download valid IERS file, using local IERS-A"
	"unable to download valid IERS file, using bundled IERS-A"

Fix IERS_Auto logic now that IERS-A is bundled #16187

Are you sure you want to change the base?

Fix IERS_Auto logic now that IERS-A is bundled #16187

Conversation

ayshih commented Mar 12, 2024 • edited

Description

github-actions bot commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

neutrinoceros commented Mar 12, 2024

ayshih commented Mar 12, 2024

pllim left a comment

Choose a reason for hiding this comment

ayshih commented Mar 12, 2024

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayshih commented Mar 13, 2024

mhvk commented Mar 13, 2024

ayshih commented Mar 14, 2024

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neutrinoceros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayshih commented Mar 12, 2024 •

edited