API: Add environment variable for behavior planned in a 2.0#23089
API: Add environment variable for behavior planned in a 2.0#23089mattip merged 4 commits intonumpy:mainfrom
Conversation
|
One thing I may was planning on pushing later is the change that gradient returns tuples (which I think is so simple there is not much need for discussion). Just to have an example. |
cead66c to
512fe42
Compare
|
Gentle ping on having an env variable as well (for behavior we would like to enable in NumPy 2)? Any thoughts on naming or the approach (or otherwise really?) |
mattip
left a comment
There was a problem hiding this comment.
This seems staightforward enough. One small nit about having two instances that reflect this state. Should the documentation mention this will go away once we release numpy2?
| npy_numpy2_behavior = PyObject_IsTrue(_is_numpy2); | ||
| Py_DECREF(_is_numpy2); | ||
| if (npy_numpy2_behavior < 0) { | ||
| return -1; |
There was a problem hiding this comment.
I think I would prefer there be one source of truth, and not have two global variables, one in C and one in Python
There was a problem hiding this comment.
OK, I guess that would make this np._get_numpy2_behavior() would add a bit of call overhead, but that probably doesn't really matter. How does that sound?
There was a problem hiding this comment.
Opted for np._using_numpy2_behavior(). Threading it out from C is a bit weird (going through numeric.py), but maybe it doesn't matter. I am now setting the variable directly in C.
The idea of the flag is not to allow to change it right now, since there may be some things where that is hard to do in general, and it doesn't seem relevant: nobody is supposed to use it besides for testing.
This change is staged for NumPy 2.0, I assume we could get away with it otherwise, but it is a nice example and probably not pressing.
| if (env != NULL && strcmp(env, "0") != 0) { | ||
| npy_numpy2_behavior = NPY_TRUE; | ||
| } | ||
|
|
There was a problem hiding this comment.
It seems coverage is not gathering statistics from the appropriate CI run? That may be OK for now but will be problematic going forward
|
@seberg What should we do with the contents of |
|
I had a hope that we would add more :). Ignore it for now, I will copy it over into the normal release notes after branching. |
|
We have branched, that is why I am asking, I'm working on the 1.25.0 release notes now. |
|
Well, its irrelevant for the 1.25 notes I think. But after clearing out the current release notes, we can copy that one fragment over to the normal place and delete the folder. |
|
That is for main, no? It is already cleared, I'll open a PR and move the note. |
|
Yeah, but let me do it, I want to basically undo all of this PR (remove the var). |
|
OK. It was #23811, I'll just close it :) |
This effectively reverts most of numpygh-23089, the array function disabling has been removed in the meantime, so the test is now not needed anymore (the test set it to 0 before). I am sad that there weren't actually any changes, but it doesn't really matter.
This effectively reverts most of numpygh-23089, the array function disabling has been removed in the meantime, so the test is now not needed anymore (the test set it to 0 before). I am sad that there weren't actually any changes, but it doesn't really matter.
The idea of the flag is not to allow to change it right now, since there may be some things where that is hard to do in general, and it doesn't seem relevant: nobody is supposed to use it besides for testing.
Maybe more for discussion, although I think it should be OK. My current thought is to have a single new process wide feature flag and not even bother with allowing to toggle it (at least for now).
I was planning to test run this with one of:
As well as enabling a single test-run. But both of those seemed to have trickier corner cases to deal with, so lets put this here to discuss.
It would be nice to do that test run including
NPY_PROMOTION_STATE=weak, but that needs some fixes and some tests to be marked for skipping or adapted depending on the setting.