-
Notifications
You must be signed in to change notification settings - Fork 300
ENH: Shared data between cube slices #2549
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -772,6 +772,23 @@ def __init__(self, data, standard_name=None, long_name=None, | |
| for cell_measure, dims in cell_measures_and_dims: | ||
| self.add_cell_measure(cell_measure, dims) | ||
|
|
||
| # When True indexing may result in a view onto the original data array, | ||
| # to avoid unnecessary copying. | ||
| self._share_data = False | ||
|
|
||
| @property | ||
| def share_data(self): | ||
| """Share cube data when slicing/indexing cube if True.""" | ||
| return self._share_data | ||
|
|
||
| @share_data.setter | ||
| def share_data(self, value): | ||
| # Realise the data if is hasn't already been as sharing lazy data is | ||
| # not right now possible or a usecase understood. | ||
| if self.has_lazy_data(): | ||
| _ = self.data | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cpelley I really disagree that if you want to share the data, you need to realise the data here at this point in the property. This is bad practice IMHO. You're effectively applying a side-effect here to suit your specific needs. Surely, if you do require to realise the data, then it should only occur in the code base where Loading the data is a BIG deal, and it's not even advertised in the doc-string. Why? ... and where's the documentation to cover this change, so that users know what's happening? No doc-string, no documentation. We really need to be always conscious of our users.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hello @bjlittle I explicitly asked for this to be included, as part of my review of the code, so I am more culpable than @cpelley I think that you are right that this should only be implemented for setting to True I have created #2584, pointed at 1.13.x, as a proposed 'bug fix' for these concerns |
||
| self._share_data = bool(value) | ||
|
|
||
| @property | ||
| def metadata(self): | ||
| """ | ||
|
|
@@ -2177,7 +2194,7 @@ def new_cell_measure_dims(cm_): | |
| try: | ||
| first_slice = next(slice_gen) | ||
| except StopIteration: | ||
| first_slice = None | ||
| first_slice = Ellipsis if self.share_data else None | ||
|
|
||
| if first_slice is not None: | ||
| data = self._my_data[first_slice] | ||
|
|
@@ -2189,8 +2206,9 @@ def new_cell_measure_dims(cm_): | |
|
|
||
| # We don't want a view of the data, so take a copy of it if it's | ||
| # not already our own. | ||
| if isinstance(data, biggus.Array) or not data.flags['OWNDATA']: | ||
| data = copy.deepcopy(data) | ||
| if not self.share_data: | ||
| if isinstance(data, biggus.Array) or not data.flags['OWNDATA']: | ||
| data = copy.deepcopy(data) | ||
|
|
||
| # We can turn a masked array into a normal array if it's full. | ||
| if isinstance(data, ma.core.MaskedArray): | ||
|
|
||
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.
@cpelley This comment doesn't make sense.