Skip to content

TST: Get rid of wcs test warnings#9010

Merged
bsipocz merged 2 commits intoastropy:masterfrom
pllim:wcs-test-warns
Oct 24, 2019
Merged

TST: Get rid of wcs test warnings#9010
bsipocz merged 2 commits intoastropy:masterfrom
pllim:wcs-test-warns

Conversation

@pllim
Copy link
Member

@pllim pllim commented Jul 17, 2019

This should get rid of most of the wcs test warnings. This is part of work for #7928 . The warnings were captured by removing the line in setup.cfg to suppress pytest warnings and then add this in the same section:

[pytest]
filterwarnings =
    error

And then run tests with python setup.py -P wcs.

[202.72053428, 47.37893142]])
footprint = w.calc_footprint(axes=axes)
assert_allclose(footprint, ref)
with pytest.raises(FITSFixedWarning):
Copy link
Member Author

Choose a reason for hiding this comment

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

For some of these tests, I need to indent the whole block or Python complains about undefined variable (that was defined inside the block). 🤷‍♀️

from astropy.tests.helper import raises, catch_warnings
from astropy import wcs
from astropy.wcs import _wcs
from astropy.wcs import _wcs # noqa
Copy link
Member Author

Choose a reason for hiding this comment

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

flake8 complains that this import is not used but if I remove it, tests fail. 🤷‍♀️

@pllim pllim force-pushed the wcs-test-warns branch 3 times, most recently from 2b4132b to 5d09ffd Compare July 17, 2019 20:31
mywcs.wcs.cdelt = cdelt

mywcs.wcs.ctype = ['RA---TAN', 'FREQ']
# Some inputs emit RuntimeWarning
Copy link
Contributor

Choose a reason for hiding this comment

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

The warnings here are because the parameters are set incorrectly.
A WCS cannot have both CD and PC defined. A WCS cannot have CD and CDELT defined.
Since the test is not meant to test the warning messages I think the input data is incorrect.
Either fix the inputs to the test and remove warnings or file an issue and I will fix it in a different PR.
I remember seeing some function code that needs to be fixed in a similar way but I have to find it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nden , I removed the handling of RuntimeWarning. Can we proceed and merge this soon?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also opened an issue at #9310 as you requested. Thanks!

@nden
Copy link
Contributor

nden commented Aug 23, 2019

@pllim Other than that one comment I think this is good.

@pllim
Copy link
Member Author

pllim commented Aug 23, 2019

@nden, thanks for the review! I think I addressed your comment. Feel free to approve and/or merge if you are happy with this.

@pllim
Copy link
Member Author

pllim commented Aug 23, 2019

I don't think mpldev failures are related.

@bsipocz
Copy link
Member

bsipocz commented Oct 24, 2019

I think that everything got addressed from the review here, so I go ahead and merge.

Thanks @pllim!

@bsipocz bsipocz merged commit 29d0328 into astropy:master Oct 24, 2019
@pllim pllim deleted the wcs-test-warns branch October 24, 2019 15:23
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.

3 participants