-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
BUG: fix a bug where wcs_info_str's results would look different in numpy 2 VS numpy 1 #16586
base: main
Are you sure you want to change the base?
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
👋 Thank you for your draft pull request! Do you know that you can use |
aae680b
to
032f52e
Compare
Meanwhile, it took me a couple iterations to not break existing tests so maybe I can draw some inspiration from these. |
…umpy 2 VS numpy 1
032f52e
to
2d2d0fe
Compare
Alright, added a (partial) test where it seemed to make sense, let me know if you think it's worth building a more complete one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but it slightly feels like one is papering over shape being set to numpy integers. Though perhaps duck typing is really all OK.
An alternative to what you have might be to have a helper function that typesets the tuple with str
instead of repr
for the items, i.e.,
def format_tuple(t):
contents = ", ".join([str(_t) for _t in t])
return f"({contents})" if len(t) != 1 else f"({contents},)"
EDIT: Could obviously adjust to take care of None
too.
@@ -36,16 +36,21 @@ def deserialize_class(tpl, construct=True): | |||
def wcs_info_str(wcs): | |||
# Overall header | |||
|
|||
if wcs.array_shape is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, fine, though it is a bit odd that array_shape
contains numpy scalars to start with - regular array.shape
just contains int
.
@@ -60,13 +65,24 @@ def wcs_info_str(wcs): | |||
'Bounds\n') | |||
# fmt: on | |||
|
|||
if wcs.pixel_bounds is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the same comment holds here.
Thinking a bit more, the current approach does seem best - we should not have to worry about the exact type used in the shape, etc. So, just the suggestion for a quick helper function remains. |
@mhvk I'm not sure that a helper function would be helpful here as we actually have 3 slightly different situations:
Trying to fit all this logic in one function wouldn't be much simpler than just implementing 3 in-place solutions, IMHO. |
for ipix in range(wcs.pixel_n_dim): | ||
# fmt: off | ||
s += (('{0:' + str(pixel_dim_width) + 'g}').format(ipix) + ' ' + | ||
('{0:' + str(pixel_nam_width) + 's}').format(wcs.pixel_axis_names[ipix] or 'None') + ' ' + | ||
(" " * 5 + str(None) if pixel_shape[ipix] is None else | ||
('{0:' + str(pixel_siz_width) + 'g}').format(pixel_shape[ipix])) + ' ' + | ||
'{:s}'.format(str(None if wcs.pixel_bounds is None else wcs.pixel_bounds[ipix]) + '\n')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this one actually a problem? Since it was passed in to str
, I would think the representation would not change. That would also remove the need for the less elegant of the overrides...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is a problem because in the application that @Cadair showcased, wcs.pixel_bounds[ipix]
is a tuple (of numpy scalars). My test actually covers this, so you may convince yourself by checking out the test without the patch :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird because str(numpy_int)
and str(int)
should still be the same (sorry, have to run, can check later).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are, but str((np.int64(1),)) == '(np.int64(1),)' != str((int(1),)) == '(1,)'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't realize wcs.pixel_bounds[ipix]
itself returned a tuple. Should have looked better...
@Cadair what do you think of this patch ? |
Description
Fixes #16585
I spent 20min trying to write a test for this but so far I couldn't find a simple example for setting up a non-trivial WCS object (e.g.
WCS()
, for which the bug isn't apparent).In particular the example test in ndcube uses ndcube API so I can't take it straight from the report.
@Cadair, any pointers ?