Skip to content

Pass atol parameter to FITSDiff #33

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

Merged
merged 4 commits into from
Aug 23, 2022
Merged

Pass atol parameter to FITSDiff #33

merged 4 commits into from
Aug 23, 2022

Conversation

svank
Copy link
Contributor

@svank svank commented May 25, 2022

I noticed that the atol parameter doesn't get passed to astropy.io.fits.diff.FITSDiff when the reference file is FITS, and so setting atol has no effect. (e.g. @pytest.mark.array_compare(file_format='fits', atol=1) still used an atol of 0.) This PR adds a test for atol, ensures atol is passed through, and mentions the atol parameter in the readme alongside rtol (which was already mentioned).

As I was adding the test, I noticed that the existing test for the rtol parameter was buggy, in that the generated and reference files both contained only values of 1.6, so rtol could be zero or any other value and the test would still pass. (Maybe the reference files were regenerated at some point, forgetting that in this particular case, the reference file must be different from the generated file to test the tolerance values.) I've modified that test by scaling up the generated values so that the test fails if rtol isn't set to a large value, to ensure that the value of rtol is being used.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I don't grok the reference_dir setup. Is there no way to generate the truth in memory at run time? Seems weird to have to copy the same file over for a new test.

@pllim pllim requested review from astrofrog and saimn May 26, 2022 00:26
@svank
Copy link
Contributor Author

svank commented May 26, 2022

Not sure if this answers your question, but since these tests are testing the machinery that compares generated data to a reference file, I think there has to be a reference file to compare to, even if only trivial data is being generated in this case. And since the reference file name is autogenerated from the test name, I think a new test needs a new copy of the file. I suppose the reference files could be programmatically generated in a temp dir as the tests run, but I'm not sure how to easily pass that temp dir to the pytest-arraydiff wrappers.

I could easily believe I'm missing a better way to do this (or misunderstanding the tests), in this weird context where the testing framework is being used to test the testing framework itself.

@pllim
Copy link
Member

pllim commented May 26, 2022

You are probably doing it right. I am not familiar with this package. 🤷

@pllim
Copy link
Member

pllim commented May 26, 2022

I had to re-enable CI in the settings. It was disabled due to inactivity. So I have to close/re-open to kick it off. Please do not be alarmed.

@pllim pllim closed this May 26, 2022
@pllim pllim reopened this May 26, 2022
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Could you add an entry in the CHANGES.rst under 0.6?

@svank
Copy link
Contributor Author

svank commented Aug 23, 2022

Done!

@pllim pllim merged commit 1af2acd into astropy:main Aug 23, 2022
@pllim
Copy link
Member

pllim commented Aug 23, 2022

Thanks, all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants