8000 MAINT: Remove the RELAXED_STRIDES_CHECKING env variable by seberg · Pull Request #21039 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 2 commits into from
Feb 13, 2022

Conversation

seberg
Copy link
Member
@seberg seberg commented Feb 11, 2022

An error is for now raised in setup.py if this is set, eventually
we 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.

@seberg seberg force-pushed the removed-relaxed-strides-checking branch from f6d9577 to ed59980 Compare February 11, 2022 20:41
@seberg
Copy link
Member Author
seberg commented Feb 11, 2022

I did not remove the DEBUG option, it has probably serves no real purpose anymore, but aside from confusing me every time I deal with "growing" an array, it also doesn't matter.

An error is for now raised in `setup.py` if this is set, eventually
we should just delete that.
@seberg seberg force-pushed the removed-relaxed-strides-checking branch from ed59980 to 0457cc7 Compare February 11, 2022 20:53
@@ -158,7 +158,6 @@ jobs:
runs-on: ubuntu-latest
env:
USE_WHEEL: 1
NPY_RELAXED_STRIDES_DEBUG: 1
Copy link
Member Author
8000

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
Copy link
Member

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).
Copy link
Member

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"?

Copy link
Member Author

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.
Copy link
Member

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)) ||
Copy link
Member

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.

Copy link
Member
@charris charris left a 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.

@charris
Copy link
Member
charris commented Feb 13, 2022

The test failure looks unrelated to this, but is bothersome.

@charris charris merged commit 4c60b32 into numpy:main Feb 13, 2022
@charris
Copy link
Member
charris commented Feb 13, 2022

Thanks Sebastian.

@seberg seberg deleted the removed-relaxed-strides-checking branch February 13, 2022 04:40
@rgommers
Copy link
Member

I did not remove the DEBUG option, it has probably serves no real purpose anymore, but aside from confusing me every time I deal with "growing" an array, it also doesn't matter.

Is anyone still attached to the NPY_RELAXED_STRIDES_DEBUG option? I found it's the last CI job not yet moved, and I'd be happy to just delete it rather than move it to Meson. A search through the issue tracker finds only two issues where this option was useful: gh-12172 and gh-15789. And neither of those two issues were found by the CI job. That's a bit marginal for running a CI job all the time.

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.

rgommers added a commit to rgommers/numpy that referenced this pull request Sep 15, 2023
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.
rgommers added a commit to rgommers/numpy that referenced this pull request Sep 15, 2023
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.
@seberg
Copy link
Member Author
seberg commented Sep 18, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAINT: Remove the NPY_RELAXED_STRIDES=0 compilation option
3 participants
0