Add copy kwarg to asarray
#991
Conversation
Codecov Report
@@ Coverage Diff @@
## release/1.2.x #991 +/- ##
==============================================
Coverage 91.80% 91.80%
==============================================
Files 65 65
Lines 10075 10152 +77
==============================================
+ Hits 9249 9320 +71
- Misses 826 832 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Thanks a lot @neosunhan ! I only have a comment about the array-API behaviour when copy=False, let me know what you think.
heat/core/factories.py
Outdated
| @@ -434,6 +434,7 @@ def array( | |||
| def asarray( | |||
| obj: Iterable, | |||
| dtype: Optional[Type[datatype]] = None, | |||
| copy: bool = False, | |||
There was a problem hiding this comment.
According to the array-API standard, asarray(copy=False) should return ValueError if a copy turns out to be necessary.
https://data-apis.org/array-api/latest/API_specification/generated/signatures.creation_functions.asarray.html
I propose to set this default to None (we won't copy unless necessary), which is basically the default numpy users expect. Then if the users specify asarray(copy=False) and copying is necessary, we can return a ValueError.
There was a problem hiding this comment.
I've changed the default and added a check in the event copy=False.
|
@JuanPedroGHM have we got a fitting label for these changes re: auto-generated changelog? I was going to suggest |
| if isinstance(copy, bool) and not copy: | ||
| if not ( | ||
| isinstance(obj, DNDarray) | ||
| and (dtype is None or dtype == obj.dtype) | ||
| and (is_split is None or is_split == obj.split) | ||
| and (device is None or device == obj.device) | ||
| ): | ||
| raise ValueError("copy is necessary") |
There was a problem hiding this comment.
The check is too broad. In some cases, numpy arrays and torch tensors can also be used without copying.
There was a problem hiding this comment.
I think the check fits better in the array function where the real logic is.
Co-authored-by: mtar <m.tarnawa@fz-juelich.de>
|
Switched base branch from |
|
Closing this as it is addressed in #1119 |
Description
Enables
copykwarg inasarray. The keyword defaults toFalsefor backward compatibility.Issue/s resolved: #990
Type of change
Due Diligence
Does this change modify the behaviour of other functions? If so, which?
no
skip ci