Repository navigation
Loose identity test of a HCOMPRESSed image - #4659
Conversation
|
@embray - would you mind taking a quick look at this? |
|
@olebole - please add a changelog entry - if it's an issue that existed for astropy 1.0.x too, then please add it in the latest 1.0.x release, otherwise in the latest 1.1.x release. |
62fdf22 to
3518cae
Compare
|
@astrofrog I added a changelog entry -- however in the master branch, since I had difficulties to rebase this. If required, I can do it again, but I would need some advise (maybe by gitter). |
embray
added
the
Affects-dev
|
We don't need a changelog entry if it just affects a test. +1 to the change otherwise. |
|
|
||
| with fits.open(self.temp('test.fits')) as hdul: | ||
| assert (hdul['SCI'].data == cube).all() | ||
| assert np.abs(hdul['SCI'].data - cube).max() < 1./15. |
There was a problem hiding this comment.
Maybe also specifically mention in a comment what you wrote in the issue concerning 1 / quantize_level. Otherwise this looks a bit mysterious.
3518cae to
39b8390
Compare
| assert (hdul['SCI'].data == cube).all() | ||
| # HCOMPRESSed images are allowed to deviate from the original by | ||
| # about 1/quantize_level of the RMS in each tile. | ||
| assert np.abs(hdul['SCI'].data - cube).max() < 1./15. |
There was a problem hiding this comment.
@olebole - I'm not too familiar with this, but isn't the quantize level 16 not 15, and does this assume the RMS is 1?
There was a problem hiding this comment.
1/16 was a bit too low, so that I assume there is an off-by-one somewhere in the docs or in my understanding.
|
@olebole - this looks ready to go in, except there is a pep8 error: Could you fix this? (I just tried editing the file on-line, but don't seem to have push access for this PR) |
|
@mhvk - labels need to be revised (I assume this is affects-release now), and needs a milestone. |
mhvk
removed
the
Affects-dev
HCOMPRESSed images are allowed to deviate from the original by about 1/quantize_level of the RMS in each tile. This patch changes the absolute identity test to a comparison with an accuracy up to the mentioned level. This fixes astropy#4647, if the cause I assume is correct. Someone with insight in HCOMPRESS should check this carefully, however.
af7c10a to
f476436
Compare
|
@mhvk I fixed this now. Thanks for the hint. |
|
CircleCI failed -- Is it related? |
|
@pllim - CircleCI failure looks unrelated, and I'm not yet sure it's a real failure or just a temporary fluke. |
|
So looks like multiple people have reviewed and we got a 👍 from Erik B, so merging. Thanks! |
…mage_stack Loose identity test of a HCOMPRESSed image
|
backported to 1.0.x in 530451c |
…mage_stack Loose identity test of a HCOMPRESSed image
…press_image_stack Loose identity test of a HCOMPRESSed image
6 participants
HCOMPRESSed images are allowed to deviate from the original by about 1/quantize_level of the RMS in each tile. This patch changes the absolute identity test to a comparison with an accuracy up to the
mentioned level.
This fixes #4647, if the cause I assume is correct. Someone with insight in
HCOMPRESS_1and cfitsio should check this carefully, however so that no real problem is going to be hidden here.