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

Prevent warnings about HIERARCH with CompImageHeader class #11404

Merged
merged 3 commits into from
Apr 9, 2021

Conversation

timj
Copy link
Contributor

@timj timj commented Mar 19, 2021

Description

Compressed image headers warn when you try to assign a HIERARCH keyword but normal image headers don't.

The underlying problem is that Card.keyword strips the HIERARCH and CompImageHeader tries to reconstruct a Card from the keyword of that card. I fix it by checking if the Card itself starts with HIERARCH already and reapply it.

This pull request is to address #11403

Fixes #11403

@timj
Copy link
Contributor Author

timj commented Mar 19, 2021

I'm not entirely sure how to test for "this thing did not warn". There are plenty of examples for warnings being issued. Do I do something like:

with assertWarns:
    run code that does not warn
    issue a warning myself

assert that I only got the warning I issued

?

@pllim
Copy link
Member

pllim commented Mar 20, 2021

The global warning config for pytest is here:

astropy/setup.cfg

Lines 115 to 141 in aed5138

filterwarnings =
error
ignore:Distutils was imported before Setuptools
ignore:unclosed file:ResourceWarning
ignore:unclosed <socket:ResourceWarning
ignore:unclosed <ssl.SSLSocket:ResourceWarning
ignore:numpy.ufunc size changed:RuntimeWarning
ignore:numpy.ndarray size changed:RuntimeWarning
ignore:Importing from numpy:DeprecationWarning:scipy
ignore:Conversion of the second argument:FutureWarning:scipy
ignore:Using a non-tuple sequence:FutureWarning:scipy
ignore:Using or importing the ABCs from 'collections':DeprecationWarning
ignore:can't resolve package from __spec__
ignore:::astropy.tests.plugins.display
ignore:::astropy.tests.disable_internet
ignore:direct construction of Asdf:pytest.PytestDeprecationWarning
ignore:Unknown pytest.mark.mpl_image_compare:pytest.PytestUnknownMarkWarning
ignore:Unknown config option:pytest.PytestConfigWarning
ignore:matplotlibrc text.usetex:UserWarning:matplotlib
# Triggered by ProgressBar > ipykernel.iostream
ignore:the imp module is deprecated:DeprecationWarning
# toolz internal deprecation warning https://github.com/pytoolz/toolz/issues/500
ignore:The toolz.compatibility module is no longer needed:DeprecationWarning
# Ignore a warning we emit about not supporting the parallel
# reading option for now, can be removed once the issue is fixed
ignore:parallel reading does not currently work, so falling back to serial
ignore:'datfix' made the change:astropy.wcs.wcs.FITSFixedWarning

With this setup, if you do nothing, all unexpected warnings, except those globally ignored, will turn into an exception. So, if you want to test that something should not raise a warning, no special code is needed. But be careful if the warning you are interested in is already ignored by pytest above.

If you want to make sure your code is raising the correct warning, use pytest.warns. More info at https://docs.pytest.org/en/stable/warnings.html . Hope this helps!

@timj
Copy link
Contributor Author

timj commented Mar 20, 2021

I've pushed a test. The failure is the samp one again.

@pllim
Copy link
Member

pllim commented Mar 20, 2021

Don't worry about the samp failure. If it keeps happening across the PRs, I'll think of something. Thanks for letting us know!

@saimn
Copy link
Contributor

saimn commented Mar 23, 2021

In that case I think it's better to test explicitly that no warning were raised, which can be done with pytest.warns(None) (you can find examples in astropy if you need).

@timj
Copy link
Contributor Author

timj commented Mar 26, 2021

I've updated the test as requested above. I assume the documentation failure is nothing to do with this change.

@pllim
Copy link
Member

pllim commented Mar 26, 2021

Yeah, matplotlib 3.4.0 release last night broke RTD.

@timj timj force-pushed the u/timj/hierarch-warn branch 4 times, most recently from 070261b to bdb45eb Compare March 26, 2021 20:55
@embray
Copy link
Member

embray commented Apr 1, 2021

Thanks for the fix. Looks good to me!

@saimn saimn added this to the v4.0.6 milestone Apr 3, 2021
@saimn
Copy link
Contributor

saimn commented Apr 3, 2021

Looks good to me too. @timj - could you rebase on master and add a changelog entry as described in https://github.com/astropy/astropy/blob/master/docs/changes/README.rst ? Thanks.

@timj
Copy link
Contributor Author

timj commented Apr 9, 2021

I've rebased and added a change log entry.

Copy link
Contributor

@saimn saimn left a comment

Choose a reason for hiding this comment

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

Thanks @timj!

@saimn saimn merged commit 7453c11 into astropy:main Apr 9, 2021
@timj timj deleted the u/timj/hierarch-warn branch February 22, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trying to set a HIERARCH keyword in a FITS Header warns about needing to use HIERARCH
4 participants