Revert changes to contiguous flags definition while creating NPY_TEST_UNSAFE_STRIDES #3162
Conversation
| @@ -31,6 +31,7 @@ from setup_common \ | |||
| MANDATORY_FUNCS, C_ABI_VERSION, C_API_VERSION | |||
|
|
|||
| ENABLE_SEPARATE_COMPILATION = (os.environ.get('NPY_SEPARATE_COMPILATION', "1") != "0") | |||
| NPY_TEST_UNSAFE_STRIDES = (os.environ.get('NPY_TEST_UNSAFE_STRIDES', "0") != "0") | |||
There was a problem hiding this comment.
Can we use a more descriptive name than "unsafe strides"? That's like 90% value judgement and 10% descriptive, reversing those proportions would be good IMO :-). Maybe NPY_EXACT_CONTIGUITY_CHECKING or NPY_TRAD_STRIDE_CHECKING or something like that? (applies throughout)
There was a problem hiding this comment.
Yes I agree... didn't think much on it to be honest, but definitely should have a better name, after all its something that will stick around a while... Could imagine also relaxed instead of exact. I don't know what that "trad" was supposed to be...
|
Thanks for the review, I addressed those comments (mostly used your new comments). And renamed to I think it mostly needs a check if the build stuff is correct. |
| @@ -15,6 +15,9 @@ | |||
|
|
|||
| # Set to True to enable multiple file compilations (experimental) | |||
| ENABLE_SEPARATE_COMPILATION = (os.environ.get('NPY_SEPARATE_COMPILATION', "1") != "0") | |||
| # Set to True to enable testing if a program can handle relaxed contiguous | |||
| # flags definition. For these | |||
There was a problem hiding this comment.
This comment appears to have been cut off in the middle of a
This flag will toggle to a new definition for the contiguous flags where only the memory layout is forced to be correct. As a particular example this means that np.ones((3,1)) can be both C and F-Contiguous and its stride[-1] can be arbitrary but the array still contiguous. The flag will also make most new arrays be created with MAX_INTP as stride so that unsafe usage of the stride will surface more commonly.
In this case, either the dimensions stride will never be used to access an element, so that it does not matter to the data alignment, or the array has a size of 0 and is thus never unaligned. Relaxed align flag is only active if NPY_RELAXED_STRIDES_CHECKING was set during compile time.
This largly reverts the changes to the flags setting api making the newer preferable unsafe flags setting available through the NPY_RELAXED_STRIDES_CHECKING eviroment variable. This variable is meant for testing if code will stop working when the flags definition is changed. The old definition is modified in some details to previously to enforce safer strides (which was not the case before). This means that ndim==1 size==1 arrays are not necessarily considered contiguous. Also empty arrays are not considered contiguous in some cases that were contiguous before, and the rule that an array can only be both C and F-contiguous if it is one (or zero) dimensional is relaxed, as it is incorrect for size <= 1 arrays.
|
OK, I think I addressed all those last things. Still would like feedback if I got the build stuff right. |
|
Looks good to me generally. I don't have any strong opinions on the build stuff but it looks fine to me. For now this sets the default to 1.7-compatible (!RELAXED), right? Should we leave it enabled by default for the first beta to get people's attention? Or are we waiting for Cython to implement the change? (Of course even if we're waiting for Cython, then we might have to enable it in a beta first to get them to start moving ;-).) This will need some serious explanation in the release notes. What sort exactly depends on whether we turn it on by default. |
|
Yes, default is back to 1.7-compatible (with the slight modifications). Whether the build stuff is right, is just a matter of being able to switch it to RELAXED easily. The doc needs some work, in release notes as well as reference guide, but if you don't mind, I would somewhat prefer to do that in a separate PR. I like the idea of enabling it in the (first) RC releases. But at least for 1.8. maybe only do it if cython stuff is fixed by then, if it isn't, can still get back to them and then be more persuasive for 1.9 :). |
|
Ok, let's add a Travis build that tests the non-default setting for this on 2.7 and 3.3, and then merge it. If there are more build tweaks or anyth 8000 ing than we can always fix them later (and no-one's complained in the two weeks since you posted this). |
|
Added the Travis build to test it. |
Revert changes to contiguous flags definition while creating NPY_TEST_UNSAFE_STRIDES
This (mostly) reverts the changes in the contiguous flags definitions. It does keep however some minor changes to make the current (lets call it safe) flags definition consistent for 0-sized, and 1-sized corner cases (which means that for these in some cases arrays previously considered contiguous are not considered contiguous anymore).
It introduces the NPY_TEST_UNSAFE_STRIDES build type enviroment variable to change the strides definition and make most new arrays be created with NPY_MAX_INTP as strides for dimensions with shape[dimension] == 1, to make bad assumptions in this case surface almost certainly while testing.
This needs documentation, but I think that can be addressed in a different pull request.
I have no idea about the building stuff, to be honest when I tested the code, I had some problems with using the enviroment variable and I edited the setup script itself, because it seemed it didn't pick it up, so someone please have a close look if there is something missing there (especially with different build methods, etc.)