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.

Sign up for GitHub

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

Jump to bottom

Fix missing .shape check for array data #14528

Merged
merged 1 commit into from
Mar 15, 2023
Merged

Fix missing .shape check for array data #14528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 10 additions & 13 deletions astropy/io/fits/hdu/image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,19 +264,16 @@ def data(self, data):
self._data_replaced = True
was_unsigned = False

if (
data is not None
and not isinstance(data, np.ndarray)
and not _is_dask_array(data)
):
# Try to coerce the data into a numpy array--this will work, on
# some level, for most objects
try:
data = np.array(data)
except Exception:
raise TypeError(
f"data object {data!r} could not be coerced into an ndarray"
)
if data is not None:
pllim marked this conversation as resolved.
Show resolved Hide resolved
if not isinstance(data, np.ndarray) and not _is_dask_array(data):
# Try to coerce the data into a numpy array--this will work, on
# some level, for most objects
try:
data = np.array(data)
except Exception: # pragma: no cover
raise TypeError(
Copy link
Member Author

@kYwzor kYwzor Mar 14, 2023 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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).

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@kYwzor kYwzor Mar 14, 2023 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

f"data object {data!r} could not be coerced into an " f"ndarray"
)

if data.shape == ():
raise TypeError(
Expand Down
9 changes: 6 additions & 3 deletions astropy/io/fits/tests/test_hdulist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,11 +547,14 @@ def test_new_hdulist_extend_keyword(self):

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"))
sci = fits.ImageHDU(data=np.array([10]))
hdul = fits.HDUList([hdu, sci])
assert "EXTEND" in hdu.header
assert hdu.header["EXTEND"] is True
hdul.writeto(self.temp("temp.fits"))
hdr = fits.getheader(self.temp("temp.fits"))
assert "EXTEND" in hdr
assert hdr["EXTEND"] is True

def test_replace_memmaped_array(self, home_is_temp):
# Copy the original before we modify it
Expand Down
5 changes: 5 additions & 0 deletions astropy/io/fits/tests/test_image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1126,6 +1126,11 @@ def test_hdu_creation_with_scalar(self):
fits.ImageHDU(data=1)
with pytest.raises(TypeError, match=msg):
fits.PrimaryHDU(data=1)
# Regression test for https://github.com/astropy/astropy/issues/14527
with pytest.raises(TypeError, match=msg):
fits.ImageHDU(data=np.array(1))
with pytest.raises(TypeError, match=msg):
fits.PrimaryHDU(data=np.array(1))
Copy link
Member

Choose a reason for hiding this comment

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

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/*",

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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. 😸

Copy link
Member

Choose a reason for hiding this comment

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

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].

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, maybe **.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?



class TestCompressedImage(FitsTestCase):
Expand Down
1 change: 1 addition & 0 deletions docs/changes/io.fits/14528.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
``ImageHDU`` now properly rejects Numpy scalars, avoiding data corruption.