Skip to content
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

MemoryView slicing contiguity bugfix #2961

Merged
merged 2 commits into from Jun 1, 2019
Merged

MemoryView slicing contiguity bugfix #2961

merged 2 commits into from Jun 1, 2019

Conversation

rjtobin
Copy link
Contributor

@rjtobin rjtobin commented May 18, 2019

This addresses #2941. The MemoryView scalar assignment code currently generates a stride 1 (ie. contiguous) loop whenever the destination of the slice is contiguous, regardless of the slice. This isn't quite right, since slices of contiguous arrays of course aren't necessarily contiguous. This patch just adds a check: if any of the indices of the slice is not simply :, then do not assume contiguity (these slices are referred to as "ToughSlices" elsewhere in the code).

One can do better than this, ie. if for a C-contiguous array the slice a[1][:] is contiguous, so the old generated code is correct and the new generated code is a little slower. Not ideal, and maybe is worth adding logic to detect these cases, but this seems a reasonable first-pass fix.

@scoder
Copy link
Contributor

scoder commented May 19, 2019

The change looks ok, but tests are missing.
Memoryview declaration tests are in tests/memoryview/memslice.pyx and usage tests are in tests/memoryview/memoryview.pyx.

@scoder
Copy link
Contributor

scoder commented May 19, 2019

Hmm, hang on. Shouldn't we rather set is_c_contig and is_f_contig to false in this case?

@rjtobin
Copy link
Contributor Author

rjtobin commented May 19, 2019

Will add tests shortly.

Hmm, hang on. Shouldn't we rather set is_c_contig and is_f_contig to false in this case?

It seemed that that the MemoryView slice itself doesn't have an is_c_contig or is_f_contig attribute, just the underlying MemoryView (which in this case is c contiguous, so I think it is correctly set to True). The current slice_iter code checks the is_c_contiguous attribute of the underlying MemoryView and then assumes the slice is also contiguous. Maybe I'm missing something though!

@rjtobin
Copy link
Contributor Author

rjtobin commented May 26, 2019

The change looks ok, but tests are missing.
Memoryview declaration tests are in tests/memoryview/memslice.pyx and usage tests are in tests/memoryview/memoryview.pyx.

I've added a couple of tests to the existing contiguous memview slice test code.

@@ -1,5 +1,5 @@
# mode: run

# tag: abcd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, that was quite careless. Removed (and rebased on top of current master).

@scoder scoder added this to the 3.0 milestone Jun 1, 2019
@scoder scoder merged commit b66a9dc into cython:master Jun 1, 2019
@scoder
Copy link
Contributor

scoder commented Jun 1, 2019

Thanks!

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.

None yet

2 participants