Skip to content
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

Remove ERFA warnings #1721

Merged
merged 11 commits into from
Mar 14, 2024
Merged

Remove ERFA warnings #1721

merged 11 commits into from
Mar 14, 2024

Conversation

aarchiba
Copy link
Contributor

@aarchiba aarchiba commented Feb 18, 2024

This PR sets pytest so that ERFA warnings become exceptions, then adjusts things so that the test suite passes. For the most part, this amounts to ignoring/suppressing ERFA warnings that are generated when the test suite (usually because of hypothesis) supplies very unusual dates to Time functions. There is at least one instance where loading the Parkes clock file requires handling MJD 0, which meant emitting an ERFA warning every time you worked with Parkes data. This has been fixed; "dubious year" warnings are no longer emitted when loading clock files. There were also some spurious ERFA warnings emitted from the astrometry code, and from bogus last corrections in old BIPM files.

The entire test suite now runs without emitting ERFA warnings.

@aarchiba
Copy link
Contributor Author

If this is helpful, a wider warning-suppression effort could be carried out - not too painful, just requires understanding which warnings come from test cases and users should see them, and which warnings come from normal situations and users shouldn't.

This has a number of advantages, including that it doesn't break `pytest -n auto`.
@aarchiba
Copy link
Contributor Author

I also replaced some random tests (that break parallel testing and won't produce reproducible failures) with hypothesis (which is intended for the purpose).

@aarchiba aarchiba added enhancement minor A minor PR that doesn't need a lot of thought awaiting review This PR needs someone to review it so it can be merged labels Feb 18, 2024
Copy link

codecov bot commented Feb 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.07%. Comparing base (bf9e15b) to head (278d80c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1721      +/-   ##
==========================================
+ Coverage   69.06%   69.07%   +0.01%     
==========================================
  Files         105      105              
  Lines       24703    24713      +10     
  Branches     4410     4412       +2     
==========================================
+ Hits        17061    17071      +10     
  Misses       6534     6534              
  Partials     1108     1108              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aarchiba
Copy link
Contributor Author

aarchiba commented Feb 19, 2024

The oldestdeps failure looks like we are triggering an internal EFRA bug that has been fixed with the upgrade from 2.0.0.3 to 2.0.1. I'm not quite sure what to do about it.

Yes, definitely something fixed in the pyerfa 2.0.0.3 -> 2.0.1 update; we seem able to trigger "System error: should not occur" from pmsafe with the old version, but not with the new.

@kerrm
Copy link
Contributor

kerrm commented Feb 28, 2024

These changes look positive to me! Two quick things:

  1. I'm pretty sure I could produce BIPM corrections using the same code that produced the ones in the tempo2 repository, and thus remove the bogus last entries. Is that a better solution than handling the <=2019 ones separately?
  2. Typo: " warnings.filterwarnings("ignore", r".*dubuious year", erfa.ErfaWarning)

if np.any(t > self.clock_file.time[-1]):
with warnings.catch_warnings():
# FIXME: could these warnings be useful?
warnings.filterwarnings("ignore", r".*dubuious year", erfa.ErfaWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the warning suppression wasn't working because of the typo, and the test suite passed anyway, clearly it wasn't needed here. (I feel better not suppressing the warning in library code unless I can clearly justify the need to.)

@aarchiba
Copy link
Contributor Author

These changes look positive to me! Two quick things:

1. I'm pretty sure I could produce BIPM corrections using the same code that produced the ones in the tempo2 repository, and thus remove the bogus last entries.  Is that a better solution than handling the <=2019 ones separately?

The global-clock-corrections repository already generates the post-2019 ones from BIPM files. But I vaguely remember there were, at least potentially, complexities in older versions (a slightly different format of the BIPM files maybe?). Or maybe I just didn't want to mess with the versions everyone was already using.

It's not ideal to have the special case in PINT, but PINT already has information about which observatory clock files come with bogus last corrections built in to it (though in a config file). And the BIPM stuff seems global/generic enough. I considered just dropping the last correction on all BIPM files - they all include a thousand-day forecast, so dropping the last of those days isn't doing much harm.

@scottransom
Copy link
Member

Looks good! Thanks a bunch!

@scottransom scottransom merged commit 870f340 into nanograv:master Mar 14, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review This PR needs someone to review it so it can be merged enhancement minor A minor PR that doesn't need a lot of thought
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants