-
Notifications
You must be signed in to change notification settings - Fork 180
JP-3982: Ignore fitsdiffs for atoca spectra #9388
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
Conversation
f5a6120 to
afc160e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9388 +/- ##
=======================================
Coverage 75.39% 75.39%
=======================================
Files 368 368
Lines 36842 36842
=======================================
Hits 27777 27777
Misses 9065 9065 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
98d6e8d to
54f31fd
Compare
|
Regression test run for just the ATOCA extras tests is here (attempt 6 for current version of tests): Full regtests here: |
| diff = FITSDiff(rtdata.output, rtdata.truth, **fitsdiff_default_kwargs) | ||
| assert diff.identical, diff.report() | ||
| if suffix == "AtocaSpectra": | ||
| # Supplemental output from atoca may have system dependent diffs |
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.
For just Ubuntu, this was passing on stable deps but failing on devdeps, right? Or is it something else now? If former, I guess we have no choice.
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.
It is still passing on stable Linux, but failing on devdeps and failing on my local Mac. The diffs are reasonable for some of the well-formed spectra contained in the file, but entirely unreasonable for some bad pixels and some empty spectra -- like many orders of magnitude relative difference. I'm not super familiar with this algorithm, but it looks to me like there is a diverging fit somewhere that is responding drastically to minor numerical changes.
I tried to see if there was a way I could diff just the reasonable extensions in the file, but there is no way to do that in the FITSDiff interface, because all the extensions have the same name.
In the end, this is just an auxiliary product that probably doesn't see much use, which is why I proposed just not diffing the product for now. If interest picks up in making this product stable and useful, we should revisit the test.
pllim
left a comment
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.
I don't know what Atoca is but it is sure not Ibiza. Yes better to get devdeps green again for other work for now. I'll let you all decide if a follow up issue is necessary lest we forgot about this. Thanks!
Resolves JP-3982
The auxiliary output from ATOCA appears to be radically different in different build environments - the fit diverges wildly in unpredictable places for Mac vs Linux, different numpy versions, etc. I think it's not reasonable to do a fitsdiff at all on this product. This PR changes the regtest to just check for existence of the output file instead of checking the contents.
Merging this should fix the last of the regtest failures with devdeps.
Tasks
Build 12.0(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see changelog readme for instructions)docs/pageokify_regteststo update the truth files