Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork11.9k
API: Introducecopy argument fornp.asarray [Array API]#25168
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
seberg left a comment
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.
Not sure what you are looking for. If we need to add thecopy kwarg toasarray, this is the right way.
The first interesting part is vetting the "no copy" logic (I don't remember if it is bad, but I definitely don't trust it without someone auditing it once more).
The other interesting part is places wherecopy=False already exists.
Fornp.array() that is likely fine (a brief search may be nice), some peoplewill be usingnp.array(..., copy=False) or actuallynp.array(x, None, False) (or a dtype)1.
The hope for those use-cases is that most of them will actually like the new meaning, and if they don't they will fail gracefully (it would be cute to mentioncopy=None andasarray() in the error message).
The trickier thing is probablyarr.astype(original, copy=False). The problem is that this is very muchvery common code and such code will definitely expect the copy to be necessary often.
Also there, you can kust replace it withcopy=None, but I am worried about the amount of churn we might create, sincecopy=False was always a best practice!
(It may be that while a best-practice in many places, it isn't nearly as common in end-user code, but I suspect it is fairly common.)
Footnotes
That may look crazy, but the the use of kwargs used to come with a large overhead and
asarraywas a Python wrapper using kwargs making it very slow for just returning the original object.↩
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ngoldbaum commentedDec 5, 2023
It looks like there are a number of places pandas uses |
ngoldbaum commentedDec 5, 2023
You can usehttps://pandas-coverage-12d2130077bc.herokuapp.com to find which pandas tests hit a particular line of code in the pandas codebase. |
311af11 to609f35fCompareseberg commentedDec 8, 2023
It seems to me that there is a worm somewhere in all of this. What is So, is the meaning of |
mtsokol commentedDec 8, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@seberg To sum up:
For (Also I reflected these changes in masked arrays and legacy matrices, let me know if I should revert any of public API changes) I updated the whole codebase to adhere to these new meanings (if there are no caveats I can address it in downstream libraries).
Hmm I see that Array API defines
|
seberg commentedDec 8, 2023
I am trying to avoid to think about it... I think it is strange to diverge the meaning, but breaking |
copy argument fornp.asarraycopy argument fornp.asarray96ce46d toa5785c7Compare9a4b7a2 to92cb661Comparemtsokol commentedDec 15, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi@seberg, I updated the PR according to our discussion during the triage call anddata-apis/array-api#721. The new semantics In the C-level code there is an updated definition of If it's Ok, I would update |
seberg commentedDec 15, 2023
I donot see it as my decision to give the OK on changing how copy behaves in But, I do not like the change. The reason why I can live with the redefinition of |
seberg commentedDec 15, 2023
In fact, we could just split out the |
d226c87 to0ede769Comparecopy argument fornp.asarraycopy argument fornp.asarray [Array API]copy argument fornp.asarray [Array API]copy argument fornp.asarray [Array API]mattip commentedFeb 21, 2024
In the triage meeting we decided to add the kwarg in a try, catch a kwarg error, and emit a deprecation error if the user's |
mtsokol commentedFeb 21, 2024
@seberg The only relevant place that I found where Classes in tests/codebase that implement |
mtsokol commentedFeb 22, 2024
Ok, I updated docs so that |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ngoldbaum left a comment
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.
Sorry for not following up quickly after you responded to my last review. I think there's still one refcounting issue. I also noticed a separate more minor issue in the docs but I think is worth addressing to reduce confusion from users.
@seberg if you have a chance, it would be nice if you could confirm you're ok with the semantics in this version of the PR just so I can make sure we're all on the same page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
seberg commentedFeb 29, 2024
I am happy with the addition. Note that it will be quite noisy unless you do not pass copy=None and take it this very slow. Code wise happy if you are. matching error exact is likely nice. Passing positionally for simplicity woukd be fine to me or using vectorcall would be best (much quicker probably and probably also simpler). |
ngoldbaum commentedFeb 29, 2024
Merging this. Thanks@mtsokol! Also thanks for getting all the NEP 56 PRs across the line! 🎆 🥳 If you are reading this because this PR broke your nightly tests, take a look at the new docs, the NEP, and the scipy and scikit-learn PRs. If there is some sort of breakage in your code that is not straightforward to deal with in a backward compatible way, please open an issue. |
rgommers commentedMar 2, 2024
Thanks for the hard work on this! Still a few things to iron out I think, xrefgh-25916. |
Uh oh!
There was an error while loading.Please reload this page.
Tracking issue:#25076
Hi@seberg,
Here's a PR with an initial work on introducing
copykeyword tonp.asarray.I started looking at the internals that we discussed during the triage meeting, but first, I wanted to try with the simplest approach: The implementation of
np.asarrayis mostly the same asnp.array- it just skips a few parameters. One of them iscopywhich is hardcoded toFalseinnp.asarray.Here I added
copywith a default valueFalse, so it's a backward compatible change. Forcopy=Trueit behaves the same way asnp.array(..., copy=True). To sum up - thecopymechanism didn't change here, only thecopykeyword is added for compatibility.WDYT?