-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
MAINT: Remove the RELAXED_STRIDES_CHECKING env variable #21039
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
Conversation
f6d9577
to
ed59980
Compare
I did not remove the DEBUG option, it |
An error is for now raised in `setup.py` if this is set, eventually we should just delete that.
ed59980
to
0457cc7
Compare
@@ -158,7 +158,6 @@ jobs: | |||
runs-on: ubuntu-latest | |||
env: | |||
USE_WHEEL: 1 | |||
NPY_RELAXED_STRIDES_DEBUG: 1 |
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.
Decided to move this to the other one (so use_wheel is just that and nothing else) and rename that one.
* and the array is contiguous if ndarray.squeeze() is contiguous. | ||
* I.e. dimensions for which `ndarray.shape[dimension] == 1` are | ||
* ignored. | ||
* A higher dimensional array is always C_CONTIGUOUS and F_CONTIGUOUS if it |
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.
Maybe "or" instead of "and"? This make is sound like the array is both, which is probably true for empty arrays, but squeezed arrays might be one of the other. Maybe rewrite?
* that for contiguous arrays strides[-1] (resp. strides[0]) always | ||
* contains the itemsize. | ||
* can be C- or F- contiguous, or neither, but not both (unless it has only | ||
* a single element). |
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 true, Maybe "one dimensional and contiguous"?
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.
It is true based on the simplified (old) rules that are given above?
* order specific stride setting is not necessary. | ||
* NOTE: The order specific stride setting is not necessary to preserve | ||
* contiguity and could be removed. However, this way the resulting | ||
* strides do currently look clearer for fortran order inputs. |
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.
Maybe "strides look better for . . ."?
*/ | ||
Py_INCREF(self); | ||
if ((PyArray_SIZE(self) > 1) && | ||
((order == NPY_CORDER && !PyArray_IS_C_CONTIGUOUS(self)) || | ||
if (((order == NPY_CORDER && !PyArray_IS_C_CONTIGUOUS(self)) || |
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.
Just check if this is correct.
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.
Couple of nits, otherwise lgtm.
The test failure looks unrelated to this, but is bothersome. |
Thanks Sebastian. |
Is anyone still attached to the Can we delete both the job and the build option? If anyone wants to keep the build option, I can integrate the build flag in one other CI job. It does not need a standalone job. |
This follows up on numpygh-21039. As noted there, this doesn't really serve any purpose anymore and historically seems to have resulted in only 2 bug reports.
This follows up on numpygh-21039. As noted there, this doesn't really serve any purpose anymore and historically seems to have resulted in only 2 bug reports.
I don't care much about keeping it, otherwise yes, it should be included in another random job not its own (I think we used to bunch up a few vars, but the other env vars went away). |
An error is for now raised in
setup.py
if this is set, eventuallywe should just delete that.
Closes gh-20220
Thought I would do this now, might also help with finally fixing up those UH-OHs, if just a little.