Fix missing .shape check for array data #14528

kYwzor · 2023-03-14T16:42:42Z

Description

This pull request is to address ImageHDU missing a necessary check for zero-dimensional data.

Fixes #14527

github-actions · 2023-03-14T16:43:31Z

astropy/io/fits/hdu/image.py

kYwzor · 2023-03-14T17:14:24Z

Very interesting that test_new_hdulist_extend_keyword is failing:

astropy/astropy/io/fits/tests/test_hdulist.py

Lines 541 to 554 in 1b41891

    
               def test_new_hdulist_extend_keyword(self): 
        
                   """Regression test for https://aeon.stsci.edu/ssb/trac/pyfits/ticket/114 
        
                   Tests that adding a PrimaryHDU to a new HDUList object updates the 
        
                   EXTEND keyword on that HDU. 
        
                   """ 
        
                   h0 = fits.Header() 
        
                   hdu = fits.PrimaryHDU(header=h0) 
        
                   sci = fits.ImageHDU(data=np.array(10)) 
        
                   image = fits.HDUList([hdu, sci]) 
        
                   image.writeto(self.temp("temp.fits")) 
        
                   assert "EXTEND" in hdu.header 
        
                   assert hdu.header["EXTEND"] is True

I can no longer access the referenced issue, but apparently this is just supposed to test if the EXTEND keyword is added to the header. However there's two weird things about it:

This is actually silently creating a corrupt FITS file, but since it is not read back it never detected the issue (basically what this PR is trying to fix)
I don't understand why the HDUList is being written to a file at all, since the assertion would pass as soon as the HDUList is created, and the file isn't being read back anyway.

I will change this test to fix this (unless I'm missing something?).

pllim · 2023-03-14T17:18:16Z

astropy/io/fits/tests/test_image.py

+        with pytest.raises(TypeError, match=msg):
+            fits.ImageHDU(data=np.array(1))
+        with pytest.raises(TypeError, match=msg):
+            fits.PrimaryHDU(data=np.array(1))


Huh, weird that this test_image.py is not being ignored by coverage. @nstarman , is this setting being ignored? You moved it in #13266 .

astropy/pyproject.toml

Line 359 in 1b41891

"*/astropy/*/tests/*",

For some reason it complains about coverage on test files while the rest of the tests are still running, but as soon as everything finishes it stops complaining. This also happened to my commit last week, but I have no clue why.

This is because we have several jobs in the same build that upload coverage reports. They don't all check the same test suite. The most comprehensive one is also the slowest, so when that one is done is when you should have a look. I won't worry about the complains while the test suite is still running. 😸

hm. This is what the coverage docs say:

Coverage.py will read from “pyproject.toml” if TOML support is available, either because you are running on Python 3.11 or later, or because you installed with the toml extra (pip install coverage[toml]). Configuration must be within the [tool.coverage] section, for example, [tool.coverage.run].

AFAIK it should be working. I see coverage[toml] in setup[cfg].

Do you think we need a ** instead of a *? Or maybe a */* since this is one more level down?

Yeah, maybe **.

codecov was happy when the change was made in #13266, so maybe those files were already not ignored before that and we would need **. To be checked with another PR ?

astropy/io/fits/tests/test_hdulist.py

pllim · 2023-03-14T17:22:41Z

test_new_hdulist_extend_keyword was moved from PyFITS over 11 years ago. Unless @embray remembers, I don't think anyone knows the original intent anymore. We no longer have access to the Trac tickets.

kYwzor · 2023-03-14T18:19:59Z

astropy/io/fits/hdu/image.py

+                try:
+                    data = np.array(data)
+                except Exception:
+                    raise TypeError(


Coverage is complaining about this TypeError, even though I did not add it. That said it is right in the sense that this TypeError is indeed never checked in any test.
More importantly: I can't think of any scenario where "data = np.array(data)" would actually raise an exception. Numpy seems to happily eat anything you throw at it, and if it is not array_like it will just create a 0-dimensional array containing the object. So I believe this part of the code is actually not needed. I will remove it.

In this case, you may use # pragma: no cover to ask coverage to stop complaining. The check goes through diff even though you might not have really touched that code.

I think I would rather delete the try-except, assuming there's no way it will trigger (I don't think it should be possible, looking over at the documentation).

That should be a separate PR, if you really want to pursue, as it is out of scope here.

Fixed in the latest commit, this PR should be ready now.

np.array([[1,2],[2,3,4]]) raises an exception, so let's just keep code as it is :)
And covering all exceptions is not easy, not done most of the time, and not in the scope of this PR, so I don't mind if Coverage is complaining.

pllim · 2023-03-14T19:04:14Z

Might need to squash but maybe we should wait for saimn first. Thanks!

saimn · 2023-03-14T20:56:06Z

Thanks for the PR, changes make sense, I forgot about Numpy scalars when doing #10041.

pllim · 2023-03-14T21:04:36Z

Thanks, @saimn ! @kYwzor , can you please squash the commits into one?

docs/changes/io.fits/14528.bugfix.rst

kYwzor · 2023-03-15T09:30:29Z

Commits are squashed.

pllim

LGTM but @saimn should look again and decide if we want to backport or not. I tentatively said we don't have to backport.

Thanks, all!

saimn

Thanks @kYwzor.

saimn · 2023-03-15T16:29:47Z

v5.3 is fine I think, it's really an edge case so backport doesn't seem needed.

kYwzor requested a review from saimn as a code owner March 14, 2023 16:42

github-actions bot added the io.fits label Mar 14, 2023

pllim reviewed Mar 14, 2023

View reviewed changes

astropy/io/fits/hdu/image.py Show resolved Hide resolved

pllim reviewed Mar 14, 2023

View reviewed changes

astropy/io/fits/tests/test_hdulist.py Outdated Show resolved Hide resolved

pllim added this to the v5.0.6 milestone Mar 14, 2023

pllim added the Bug label Mar 14, 2023

pllim modified the milestones: v5.0.6, v5.3 Mar 14, 2023

kYwzor commented Mar 14, 2023

View reviewed changes

saimn reviewed Mar 14, 2023

View reviewed changes

docs/changes/io.fits/14528.bugfix.rst Outdated Show resolved Hide resolved

Fix missing .shape check for array data

cbd57cc

kYwzor force-pushed the fix-corrupt-images branch from 03ab1d1 to cbd57cc Compare March 15, 2023 09:28

pllim approved these changes Mar 15, 2023

View reviewed changes

saimn approved these changes Mar 15, 2023

View reviewed changes

saimn merged commit 5408748 into astropy:main Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing .shape check for array data #14528

Fix missing .shape check for array data #14528

kYwzor commented Mar 14, 2023

github-actions bot commented Mar 14, 2023 •

edited by pllim

kYwzor commented Mar 14, 2023 •

edited

pllim Mar 14, 2023

kYwzor Mar 14, 2023

pllim Mar 14, 2023

nstarman Mar 14, 2023

pllim Mar 14, 2023

nstarman Mar 14, 2023

saimn Mar 14, 2023

pllim commented Mar 14, 2023

kYwzor Mar 14, 2023 •

edited

pllim Mar 14, 2023

kYwzor Mar 14, 2023

pllim Mar 14, 2023

kYwzor Mar 14, 2023 •

edited

saimn Mar 14, 2023

pllim commented Mar 14, 2023

saimn commented Mar 14, 2023

pllim commented Mar 14, 2023

kYwzor commented Mar 15, 2023

pllim left a comment

saimn left a comment

saimn commented Mar 15, 2023

Fix missing .shape check for array data #14528

Fix missing .shape check for array data #14528

Conversation

kYwzor commented Mar 14, 2023

Description

github-actions bot commented Mar 14, 2023 • edited by pllim

kYwzor commented Mar 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Mar 14, 2023

kYwzor Mar 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kYwzor Mar 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Mar 14, 2023

saimn commented Mar 14, 2023

pllim commented Mar 14, 2023

kYwzor commented Mar 15, 2023

pllim left a comment

Choose a reason for hiding this comment

saimn left a comment

Choose a reason for hiding this comment

saimn commented Mar 15, 2023

github-actions bot commented Mar 14, 2023 •

edited by pllim

kYwzor commented Mar 14, 2023 •

edited

kYwzor Mar 14, 2023 •

edited

kYwzor Mar 14, 2023 •

edited