-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix IERS_Auto logic now that IERS-A is bundled #16187
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -165,9 +165,9 @@ | |||||
|
||||||
auto_download = _config.ConfigItem( | ||||||
True, | ||||||
"Enable auto-downloading of the latest IERS data. If set to False " | ||||||
"then the local IERS-B file will be used by default (even if the " | ||||||
"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). " | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor, but to me, "already" may suggest that this could have happened while running the current process.
Suggested change
|
||||||
"This parameter also controls whether internet resources will be " | ||||||
"queried to update the leap second table if the installed version is " | ||||||
"out of date. Default is True.", | ||||||
|
@@ -191,7 +191,7 @@ | |||||
["error", "warn", "ignore"], | ||||||
"IERS behavior if the range of available IERS data does not " | ||||||
"cover the times when converting time scales, potentially leading " | ||||||
"to degraded accuracy.", | ||||||
"to degraded accuracy. Applies only when using IERS-B data on its own.", | ||||||
) | ||||||
system_leap_second_file = _config.ConfigItem("", "System file with leap seconds.") | ||||||
iers_leap_second_auto_url = _config.ConfigItem( | ||||||
|
@@ -545,9 +545,9 @@ | |||||
|
||||||
Notes | ||||||
----- | ||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
version can be downloaded from ``iers.IERS_A_URL`` or ``iers.IERS_A_URL_MIRROR``. | ||||||
See ``iers.__doc__`` for instructions on use in ``Time``, etc. | ||||||
""" | ||||||
|
||||||
iers_table = None | ||||||
|
@@ -758,6 +758,10 @@ | |||||
""" | ||||||
Provide most-recent IERS data and automatically handle downloading | ||||||
of updated values as necessary. | ||||||
|
||||||
The returned table combines the IERS-A and IERS-B files, with the data | ||||||
in the IERS-B file considered to be official values and thus superseding | ||||||
values from the IERS-A file at the same times. | ||||||
""" | ||||||
|
||||||
iers_table = None | ||||||
|
@@ -772,8 +776,8 @@ | |||||
(or non-existent) then it will be downloaded over the network and cached. | ||||||
|
||||||
If the configuration setting ``astropy.utils.iers.conf.auto_download`` | ||||||
is set to False then ``astropy.utils.iers.IERS()`` is returned. This | ||||||
is normally the IERS-B table that is supplied with astropy. | ||||||
is set to False then the bundled IERS-A table will be used rather than | ||||||
any downloaded version of the IERS-A table. | ||||||
|
||||||
On the first call in a session, the table will be memoized (in the | ||||||
``iers_table`` class attribute), and further calls to ``open`` will | ||||||
|
@@ -786,7 +790,7 @@ | |||||
|
||||||
""" | ||||||
if not conf.auto_download: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the alternative path, which would preserve anything downloaded, would simply be to remove this stanza and in the Am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussion concluded we should leave this for follow-up |
||||||
cls.iers_table = IERS_B.open() | ||||||
cls.iers_table = cls.read() | ||||||
return cls.iers_table | ||||||
|
||||||
all_urls = (conf.iers_auto_url, conf.iers_auto_url_mirror) | ||||||
|
@@ -817,8 +821,8 @@ | |||||
# Issue a warning here, perhaps user is offline. An exception | ||||||
# will be raised downstream if actually trying to interpolate | ||||||
# predictive values. | ||||||
warn("unable to download valid IERS file, using local IERS-B", IERSWarning) | ||||||
cls.iers_table = IERS_B.open() | ||||||
warn("unable to download valid IERS file, using local IERS-A", IERSWarning) | ||||||
cls.iers_table = cls.read() | ||||||
|
||||||
return cls.iers_table | ||||||
|
||||||
|
@@ -932,7 +936,7 @@ | |||||
IERS-A has IERS-B values included, but for reasons unknown these | ||||||
do not match the latest IERS-B values (see comments in #4436). | ||||||
Here, we use the bundled astropy IERS-B table to overwrite the values | ||||||
in the downloaded IERS-A table. | ||||||
in the IERS-A table. | ||||||
""" | ||||||
iers_b = IERS_B.open() | ||||||
# Substitute IERS-B values for existing B values in IERS-A table | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -25,27 +25,9 @@ | |||||
|
||||||
FILE_NOT_FOUND_ERROR = getattr(__builtins__, "FileNotFoundError", OSError) | ||||||
|
||||||
try: | ||||||
iers.IERS_A.open("finals2000A.all") # check if IERS_A is available | ||||||
except OSError: | ||||||
HAS_IERS_A = False | ||||||
else: | ||||||
HAS_IERS_A = True | ||||||
|
||||||
IERS_A_EXCERPT = get_pkg_data_filename(os.path.join("data", "iers_a_excerpt")) | ||||||
|
||||||
|
||||||
def setup_module(): | ||||||
# Need auto_download so that IERS_B won't be loaded and cause tests to | ||||||
# fail. Files to be downloaded are handled appropriately in the tests. | ||||||
iers.conf.auto_download = True | ||||||
|
||||||
|
||||||
def teardown_module(): | ||||||
# This setting is to be consistent with astropy/conftest.py | ||||||
iers.conf.auto_download = False | ||||||
|
||||||
|
||||||
class TestBasic: | ||||||
"""Basic tests that IERS_B returns correct values""" | ||||||
|
||||||
|
@@ -216,7 +198,6 @@ def test_simple(self): | |||||
assert len(iers_tab[:2]) == 2 | ||||||
|
||||||
|
||||||
@pytest.mark.skipif(not HAS_IERS_A, reason="requires IERS_A") | ||||||
class TestIERS_A: | ||||||
@classmethod | ||||||
def teardown_class(self): | ||||||
|
@@ -261,6 +242,14 @@ def setup_class(self): | |||||
self.iers_a_url_2 = Path(self.iers_a_file_2).as_uri() | ||||||
self.t = Time.now() + TimeDelta(10, format="jd") * np.arange(self.N) | ||||||
|
||||||
# This group of tests requires auto downloading to be on | ||||||
iers.conf.auto_download = True | ||||||
|
||||||
# auto_download = False is tested in test_IERS_B_parameters_loading_into_IERS_Auto() | ||||||
|
||||||
def teardown_class(self): | ||||||
iers.conf.auto_download = False | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
||||||
def teardown_method(self, method): | ||||||
"""Run this after every test.""" | ||||||
iers.IERS_Auto.close() | ||||||
|
@@ -310,12 +299,6 @@ def test_auto_max_age_minimum(self): | |||||
iers_table = iers.IERS_Auto.open() | ||||||
_ = iers_table.ut1_utc(self.t.jd1, self.t.jd2) | ||||||
|
||||||
def test_no_auto_download(self): | ||||||
with iers.conf.set_temp("auto_download", False): | ||||||
t = iers.IERS_Auto.open() | ||||||
assert type(t) is iers.IERS_B | ||||||
|
||||||
@pytest.mark.remote_data | ||||||
def test_simple(self): | ||||||
with iers.conf.set_temp("iers_auto_url", self.iers_a_url_1): | ||||||
dat = iers.IERS_Auto.open() | ||||||
|
@@ -380,8 +363,10 @@ def test_simple(self): | |||||
assert dat["MJD"][-1] == (57539.0 + 60) * u.d | ||||||
|
||||||
|
||||||
@pytest.mark.remote_data | ||||||
def test_IERS_B_parameters_loading_into_IERS_Auto(): | ||||||
# Double-check that auto downloading is off | ||||||
assert not iers.conf.auto_download | ||||||
Comment on lines
+367
to
+368
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of a guard clause, we could use a pytest fixture, or a context manager here. Any reason not to ? |
||||||
|
||||||
A = iers.IERS_Auto.open() | ||||||
B = iers.IERS_B.open() | ||||||
|
||||||
|
@@ -445,35 +430,31 @@ def test_iers_b_dl(): | |||||
iers.IERS_B.close() | ||||||
|
||||||
|
||||||
@pytest.mark.remote_data | ||||||
def test_iers_out_of_range_handling(tmp_path): | ||||||
# Make sure we don't have IERS-A data available anywhere | ||||||
with set_temp_cache(tmp_path): | ||||||
iers.IERS_A.close() | ||||||
iers.IERS_Auto.close() | ||||||
iers.IERS.close() | ||||||
def test_iers_b_out_of_range_handling(): | ||||||
# The following error/warning applies only to IERS_B, not to the default IERS_Auto | ||||||
with iers.earth_orientation_table.set(iers.IERS_B.open()): | ||||||
now = Time.now() | ||||||
with iers.conf.set_temp("auto_download", False): | ||||||
# Should be fine with built-in IERS_B | ||||||
(now - 300 * u.day).ut1 | ||||||
|
||||||
# Default is to raise an error | ||||||
match = r"\(some\) times are outside of range covered by IERS table" | ||||||
with pytest.raises(iers.IERSRangeError, match=match): | ||||||
(now + 100 * u.day).ut1 | ||||||
# Should be fine with bundled IERS-B | ||||||
(now - 300 * u.day).ut1 | ||||||
|
||||||
with iers.conf.set_temp("iers_degraded_accuracy", "warn"): | ||||||
with pytest.warns(iers.IERSDegradedAccuracyWarning, match=match): | ||||||
(now + 100 * u.day).ut1 | ||||||
# Default is to raise an error | ||||||
match = r"\(some\) times are outside of range covered by IERS table" | ||||||
with pytest.raises(iers.IERSRangeError, match=match): | ||||||
(now + 100 * u.day).ut1 | ||||||
|
||||||
with iers.conf.set_temp("iers_degraded_accuracy", "ignore"): | ||||||
with iers.conf.set_temp("iers_degraded_accuracy", "warn"): | ||||||
with pytest.warns(iers.IERSDegradedAccuracyWarning, match=match): | ||||||
(now + 100 * u.day).ut1 | ||||||
|
||||||
with iers.conf.set_temp("iers_degraded_accuracy", "ignore"): | ||||||
(now + 100 * u.day).ut1 | ||||||
|
||||||
|
||||||
@pytest.mark.remote_data | ||||||
def test_iers_download_error_handling(tmp_path): | ||||||
# Make sure we don't have IERS-A data available anywhere | ||||||
with set_temp_cache(tmp_path): | ||||||
# Make sure an IERS-A table isn't already loaded | ||||||
with set_temp_cache(tmp_path), iers.conf.set_temp("auto_download", True): | ||||||
iers.IERS_A.close() | ||||||
iers.IERS_Auto.close() | ||||||
iers.IERS.close() | ||||||
|
@@ -485,7 +466,7 @@ def test_iers_download_error_handling(tmp_path): | |||||
with iers.conf.set_temp("iers_auto_url_mirror", "https://google.com"): | ||||||
with pytest.warns(iers.IERSWarning) as record: | ||||||
with iers.conf.set_temp("iers_degraded_accuracy", "ignore"): | ||||||
(now + 100 * u.day).ut1 | ||||||
(now + 400 * u.day).ut1 | ||||||
|
||||||
assert len(record) == 3 | ||||||
assert str(record[0].message).startswith( | ||||||
|
@@ -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" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this right ?
Suggested change
|
||||||
) | ||||||
|
||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
``IERS_Auto.open()`` now always returns a table of type ``IERS_Auto`` that | ||
contains the combination of IERS-A and IERS-B data, even if automatic | ||
updating of the IERS-A file is disabled or if downloading the new file fails. | ||
Previously, under those conditions, it would return a table of a different type | ||
(``IERS_B``) with only IERS-B data. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fixed the unintended behavior where the IERS-A file bundled in ``astropy-iers-data`` would be ignored if automatic updating of the IERS-A file were disabled or if downloading the new file failed. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,8 +9,7 @@ Introduction | |||||
|
||||||
A number of Astropy's tools work with data sets that are either awkwardly | ||||||
large (e.g., `~astropy.coordinates.solar_system_ephemeris`) or | ||||||
regularly updated (e.g., `~astropy.utils.iers.IERS_B`) or both | ||||||
(e.g., `~astropy.utils.iers.IERS_A`). This kind of | ||||||
regularly updated (e.g., `~astropy.utils.iers.IERS_A`). This kind of | ||||||
data - authoritative data made available on the Web, and possibly updated | ||||||
from time to time - is reasonably common in astronomy. The Astropy Project therefore | ||||||
provides some tools for working with such data. | ||||||
|
@@ -235,9 +234,8 @@ adding files to an existing cache directory. | |||||
If your application needs IERS data specifically, you can download the | ||||||
appropriate IERS table, covering the appropriate time span, by any means you | ||||||
find convenient. You can then load this file into your application and use the | ||||||
resulting table rather than `~astropy.utils.iers.IERS_Auto`. In fact, the IERS | ||||||
B table is small enough that a version (not necessarily recent) is bundled with | ||||||
``astropy`` as ``astropy.utils.iers.IERS_B_FILE``. Using a specific non-automatic | ||||||
resulting table rather than `~astropy.utils.iers.IERS_Auto`. | ||||||
Using a specific non-automatic | ||||||
table also has the advantage of giving you control over exactly which version | ||||||
of the IERS data your application is using. See also :ref:`iers-working-offline`. | ||||||
|
||||||
|
@@ -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 | ||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
|
||||||
the one already bundled in the ``astropy-iers-data`` package. To update the | ||||||
IERS-A table, do one of the following: | ||||||
|
||||||
* Upgrade the ``astropy-iers-data`` package. | ||||||
* Write a simple script that sets ``astropy.utils.iers.conf.auto_download = | ||||||
True`` and then accesses all cached resources your code will need, | ||||||
including source name lookups and IERS tables. Run it on the head node from | ||||||
time to time (frequently enough to beat the timeout | ||||||
``astropy.utils.iers.conf.auto_max_age``, which defaults to 30 days) to | ||||||
ensure all data is up to date. | ||||||
* Set ``astropy.utils.iers.conf.auto_download = False`` in your code and set | ||||||
``astropy.utils.iers.conf.iers_degraded_accuracy`` to either ``'warn'`` | ||||||
or ``'ignore'``. These prevent the normal exception that occurs if a | ||||||
time conversion falls outside the bounds of available (local) IERS data. | ||||||
**WARNING**: only use this option if your application does not need full | ||||||
accuracy time conversions. | ||||||
|
||||||
* Make an Astropy config file (see :ref:`astropy_config`) that sets | ||||||
``astropy.utils.iers.conf.auto_download = False`` so that the worker jobs will | ||||||
not suddenly notice an out-of-date table all at once and frantically attempt | ||||||
to download it. | ||||||
|
||||||
* Optionally, in this file, set ``astropy.utils.data.conf.allow_internet = False`` to | ||||||
prevent any attempt to download any file from the worker nodes; if you do this, | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we might want to rename this variable
ctx
now. More importantly: thanks, seeing this test being simplified back is a nice surprise !