Provide .copy methods for MemDataset and MemDiskGroup
#236
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ljgray
changed the title
ljgray
force-pushed
the
ljg/dataset-from-dataset
branch
8 times, most recently
from
190b975
to
2761eba
Compare
ljgray
changed the title
ljgray
force-pushed
the
ljg/dataset-from-dataset
branch
from
2761eba
to
d3c7a30
Compare
ljgray
changed the title
ljgray
force-pushed
the
ljg/dataset-from-dataset
branch
8 times, most recently
from
572cddc
to
42045a0
Compare
ljgray
commented
Mar 14, 2023
ljgray
commented
Mar 14, 2023
ljgray
force-pushed
the
ljg/dataset-from-dataset
branch
3 times, most recently
from
a1a5d00
to
642d3f0
Compare
Provides deep_group_copy with arguments to actually deep copy datasets, with an option to shallow copy 'shared' datasets. Also, allow distributed datasets to be copied only in the memory -> memory case.
ljgray
force-pushed
the
ljg/dataset-from-dataset
branch
from
642d3f0
to
42ec637
Compare
ljgray
changed the title
.copy methods for MemDataset and MemDiskGroup
ljgray
changed the title
.copy methods for MemDataset and MemDiskGroup.copy methods for MemDataset and MemDiskGroup
kiyo-masui
reviewed
Mar 15, 2023
There was a problem hiding this comment.
I've given this a full read, and it looks good to me. I'll leave final approval to @jrs65 since he is much more up-to-date on this code than I am at this point.
ljgray
force-pushed
the
ljg/dataset-from-dataset
branch
from
42ec637
to
5c9eef2
Compare
ljgray
force-pushed
the
ljg/dataset-from-dataset
branch
from
5c9eef2
to
0a7a897
Compare
jrs65
approved these changes
Mar 16, 2023
Specifically, this lets us compare objects such as lists and dicts which contain numpy ndarrays (or subclasses) which are equal elementwise but are not actually the same object (such as after a deep copy).
ljgray
force-pushed
the
ljg/dataset-from-dataset
branch
from
0a7a897
to
5d693a5
Compare
ljgray
force-pushed
the
ljg/dataset-from-dataset
branch
from
5d693a5
to
860004a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR provides
.copymethods forMemDatasetandMemDiskGroupclasses.MemDataset.copycan optionally do a deep or shallow copy (deep is default) and set the memory layout of the underlying data (if it is a numpy array). By default, this is the same layout as the dataset being copied.MemDiskGroup.copydeep copies all datasets by default and provides asharedargument. This is implemented via thedeep_group_copymethod, which is extended to optionally deep copy datasets (except those that are shared) and copy distributed arrays only in the memory -> memory case. I made considerations based on discussion in #190.This PR also provides a a tool to check two objects which may contain numpy arrays for equality (ex: a list of arrays, dict with array arguments, etc...). For example, deep copying a list which contains a numpy array would result in an error when comparing the dicts, as the numpy arrays would not be the same object and would fall back on elementwise comparison. Since
list.__eq__won't callall()on the numpy array after elementwise comparison, the comparison would fail. The same thing happens if the outer container is a dict.Finally, the PR adds a check for matching
dtypewhen comparing twoMemDatasets. I'm undecided if this is a good idea or not.NOTE: I've done a fair bit of testing and I think this is tentatively ready for review. I do still want to try to break it a bit more thoroughly. I've tagged a few possibly relevant people for review but it's fine if this waits for awhile