Even if IERS_Auto predictive values are stale, do not download IERS-A if downloading is disabled by ayshih · Pull Request #17387 · astropy/astropy · GitHub
Skip to content

Even if IERS_Auto predictive values are stale, do not download IERS-A if downloading is disabled - #17387

Merged
mhvk merged 1 commit into
astropy:mainfrom
ayshih:disable_download
Nov 13, 2024
Merged

mhvk merged 1 commit into
astropy:mainfrom
ayshih:disable_download

Conversation

@ayshih

@ayshih ayshih commented Nov 13, 2024

Copy link
Copy Markdown
Contributor

This PR fixes the root bug revealed by #17359: the code that identifies stale IERS_Auto predictive values would always try to download a more up-to-date IERS-A even if downloading was disabled (auto_download=False).

This bug was essentially introduced by #16187, because previously auto_download=False would ignore the bundled IERS-A entirely, stale or not.

@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor

👋 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?

@ayshih

ayshih commented Nov 13, 2024

Copy link
Copy Markdown
Contributor Author

Even though this test fixes the bug that prompted #17359, it does not mean that #17377 can be reverted, because we still need those tests to pass even when interpolating off of stale IERS_Auto predictive values. Setting auto_max_age=None as #17377 does is the expedient approach to prevent the error.

@neutrinoceros neutrinoceros 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.

LGTM. Anything else you wanted to do before undrafting ?

@neutrinoceros neutrinoceros added this to the v7.0.0 milestone Nov 13, 2024
@pllim pllim modified the milestones: v7.0.0, v6.1.7, v7.0.1 Nov 13, 2024
@pllim

pllim commented Nov 13, 2024

Copy link
Copy Markdown
Member

To really test this, do we have to undo the setting added in #17377 ?

@ayshih
ayshih marked this pull request as ready for review November 13, 2024 14:12
@neutrinoceros

Copy link
Copy Markdown
Contributor

@pllim I'm confused about milestoning here: shouldn't this go to 7.0.0 ?

@ayshih

ayshih commented Nov 13, 2024

Copy link
Copy Markdown
Contributor Author

Anything else you wanted to do before undrafting ?

Just added a bit of polish to the test and comment.

To really test this, do we have to undo the setting added in #17377 ?

Nope, this bugfix is tested by the additional test in this PR. The coordinate tests fixed by #17377 still need to be fixed in that fashion, with their purpose to test coordinates code, not IERS code.

shouldn't this go to 7.0.0 ?

Yes, please put this in 7.0 if possible.

@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.

Looks good -- thanks, @ayshih, for getting to the root of the problem!

@mhvk

mhvk commented Nov 13, 2024

Copy link
Copy Markdown
Contributor

I'll enable auto-merge. I don't think 7.0.0 is possible anymore, unless the release team decides to rebranch it from 7.0.x.

@mhvk
mhvk enabled auto-merge November 13, 2024 14:22
@mhvk
mhvk merged commit 7306058 into astropy:main Nov 13, 2024
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Nov 13, 2024
…ale, do not download IERS-A if downloading is disabled
@pllim

pllim commented Nov 13, 2024

Copy link
Copy Markdown
Member

Milestone during RC period is confusing for me as well. Not sure where @saimn is going to tag for release if we are not doing RC2.

pllim added a commit that referenced this pull request Nov 13, 2024
…387-on-v7.0.x

Backport PR #17387 on branch v7.0.x (Even if IERS_Auto predictive values are stale, do not download IERS-A if downloading is disabled)
@saimn

saimn commented Nov 13, 2024

Copy link
Copy Markdown
Contributor

@pllim pllim modified the milestones: v7.0.1, v7.0.0 Nov 13, 2024
@ayshih
ayshih deleted the disable_download branch November 14, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants