-
-
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 1 commit
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.", | ||||||
|
@@ -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 th 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 @@ | ||
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. | ||
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 think we should add what it is now! Maybe
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. Overhauled text |
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. |
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 !