-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Sliceable MetaData Class #455
base: main
Are you sure you want to change the base?
Conversation
Refactor involved moving comments and axes out of the core values and putting them into their own dictionaries. This removed cascading consequences for self.values(), self.__setitem__, etc.
Hello @DanRyanIrish! Thanks for updating this PR.
Comment last updated at 2021-11-18 14:32:19 UTC |
Also put in overwrite error check to Meta.__setitem__ if axis is not None. This way the value shape and axes cannot easily be corrupted accidentally.
Only store entries in the comments and axes dicts that aren't None. This saves a lot of space and requires the get method be used when looking for comment and axes values to avoid errors is that key doesn't have a comment or axis.
@nabobalis Could you give this PR a review? It should be very similar if not the same as what's in your sunraster PR. |
@Cadair this PR is ready for review. To pre-empt possible concerns regarding DKIST tools, the introduction of this new class does not require that metadata be sliceable. Instead it allows for it if desired. In order for the Not all entries in the Therefore, as far as I understand, this new class should not threaten your assumption in the DKIST tools that |
@Cadair, this is ready to merge as far as I can see. Do you still want more time to look at it given your DKIST concerns? As I above, I don't think this causes an issues as the new functionalities are optional and don't have to change previous behaviour. |
Currently creates a bug in NDMeta.rebin.
@Cadair, except for narrative docs, I think this PR now addresses all but one minor comment of yours. Unless you'd like to look at it again, I will merge it once I've added narrative docs. |
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.
Looking good, a few more comments. I am happy to give this a final review when you are ready.
new_shape = new_meta.data_shape | ||
for i, axis_item in enumerate(item): | ||
if isinstance(axis_item, numbers.Integral): | ||
dropped_axes[i] = True | ||
elif isinstance(axis_item, slice): | ||
start = axis_item.start | ||
if start is None: | ||
start = 0 | ||
if start < 0: | ||
start = data_shape[i] - start | ||
stop = axis_item.stop | ||
if stop is None: | ||
stop = data_shape[i] | ||
if stop < 0: | ||
stop = data_shape[i] - stop | ||
new_shape[i] = stop - start | ||
else: | ||
raise TypeError("Unrecognized slice type. " | ||
"Must be an int, slice and tuple of the same.") |
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 think you might basically be able to replace all of this (other than the calculations of dropped_axes
with this:
new_shape = np.broadcast_to(1, new_meta.data_shape)[item].shape
which is equivalent to np.zeros(shape)[view].shape
but without allocating the whole array.
Thanks to @astrofrog for this 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.
As in:
new_shape = new_meta.data_shape | |
for i, axis_item in enumerate(item): | |
if isinstance(axis_item, numbers.Integral): | |
dropped_axes[i] = True | |
elif isinstance(axis_item, slice): | |
start = axis_item.start | |
if start is None: | |
start = 0 | |
if start < 0: | |
start = data_shape[i] - start | |
stop = axis_item.stop | |
if stop is None: | |
stop = data_shape[i] | |
if stop < 0: | |
stop = data_shape[i] - stop | |
new_shape[i] = stop - start | |
else: | |
raise TypeError("Unrecognized slice type. " | |
"Must be an int, slice and tuple of the same.") | |
new_shape = np.broadcast_to(1, new_meta.data_shape)[item].shape | |
dropped_axes = np.array([True if isinstance(axis_item, numbers.Integral) else False for axis_item in item], dtype=bool) |
# If value cannot be sliced by fancy slicing, convert it | ||
# it to an array, slice it, and then if necessary, convert | ||
# it back to its original type. | ||
new_value = (np.asanyarray(value)[new_item]) | ||
if hasattr(new_value, "__len__"): | ||
new_value = type(value)(new_value) |
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.
Did we say we were going to nuke this bit?
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 left a comment about this somewhere after our talk. I realised that without this, NDMeta
could not support dropping axes as part of slicing if it contained lists or tuples as axis-aligned metadata. So as ugly as it is, I think it's better to keep it.
# If meta is axis-aware, make it to have same shape as cube. | ||
if isinstance(self.meta, NDMetaABC): | ||
self.meta.data_shape = self.shape |
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.
Should this be in the setter for .meta
? Do we have a setter for .meta
?
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.
We don't. We outsource that to NDData
.
Co-authored-by: Stuart Mumford <[email protected]>
And other minor changes suggested by code review.
Thanks for the review @Cadair. I've addressed your comments with a couple exceptions where I ask for further clarifications. |
7550d1e
to
e15e2ad
Compare
Description
This PR introduces a sliceable
Meta
class based ondict
, which allows metadata to be associated with data axes and the whole class to be sliced using the normal Python slicing API. Metadata not associated with any axes is not touched by the slicing mechanism.This PR introduces two types of axis-aware metadata: axis-aligned and grid-aligned. Axis-aligned metadata gives one value for each of its associated axes, and is scalar or a string if only associated with one axis. This type of metadata is only changed if the axis/axes within which it is associated is dropped by slicing. In this case, the values corresponding to the dropped axes are also dropped.
Grid-aligned metadata provides a value for each data array element in its associated axis/axes. This metadata cannot be scalar. Since this metadata mirrors at least part of the data array, it is sliced as if it were data, and its shape is kept consistent with its associated data axes. If all its axes are sliced away, the scalar value at the row/column location at which it was sliced is kept, and its axis-awareness is dropped, i.e. it becomes a piece of normal non-axis-aware scalar metadata.
API Summary
To Do
__delitem__
and removeremove
method.NDMeta.data_shape
which verifies that it's compatible with pre-existing axis- and grid-aligned metadata.