Change structure_tensor to use row/col-notation instead of x/y. #4841
Conversation
|
Hello @coreysharris! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-07-12 15:52:26 UTC |
| 'set this explicitly.', category=FutureWarning, stacklevel=2) | ||
| order = 'xy' | ||
| else: | ||
| order = 'rc' |
There was a problem hiding this comment.
This line isn't covered because it's not meant to be hit yet.
---------- coverage: platform darwin, python 3.8.3-final-0 -----------
Name Stmts Miss Cover Missing
----------------------------------------------------------------------------
skimage/feature/corner.py 211 1 99% 109
skimage/feature/corner.py
Outdated
| if len(S_elems) == 3: # Use fast Cython code for 2D | ||
| eigs = np.array(_image_orthogonal_matrix22_eigvals(*S_elems)) | ||
| else: | ||
| matrices = _symmetric_image(S_elems) | ||
| # eigvalsh returns eigenvalues in increasing order. We want decreasing | ||
| eigs = np.linalg.eigvalsh(matrices)[..., ::-1] | ||
| leading_axes = tuple(range(eigs.ndim - 1)) | ||
| eigs = np.transpose(eigs, (eigs.ndim - 1,) + leading_axes) | ||
| return eigs |
There was a problem hiding this comment.
Stolen from hessian_matrix_eigenvals because we need it for the structure tensor too.
| image = S_elems[0] | ||
| symmetric_image = np.zeros(image.shape + (image.ndim, image.ndim)) | ||
| for idx, (row, col) in \ | ||
| enumerate(combinations_with_replacement(range(image.ndim), 2)): | ||
| symmetric_image[..., row, col] = S_elems[idx] | ||
| symmetric_image[..., col, row] = S_elems[idx] | ||
| return symmetric_image |
There was a problem hiding this comment.
This was _hessian_matrix_image, but again we needed it elsewhere.
| derivatives = _compute_derivatives(image, mode=mode, cval=cval) | ||
|
|
||
| if order == 'rc': | ||
| derivatives = reversed(derivatives) |
There was a problem hiding this comment.
I'm not 100% sure if this is correct in 3D/nD. We can worry about that in the 3D/nD PR, though.
For example, mode 'xy' (vs. 'yz') in NumPy's meshgrid only swaps the order of the first two while the rest remain the same. See the Notes section here. This would instead reverse all axes for "xy" vs "rc". That may be correct, but I don't recall offhand what is done in other functions in skimage.
There was a problem hiding this comment.
This type of potential confusion is why I am keen to see the switch to rc convention as that seems the easiest to follow
There was a problem hiding this comment.
@grlee77 I suggest that we do not support 'xy' for ndim>2. It is only a temporary crutch until everything is rc.
There was a problem hiding this comment.
I suggest that we do not support 'xy' for ndim>2
Okay, then perhaps we should raise an error for order = 'xy' when ndim != 2
There was a problem hiding this comment.
Awesome! That will make the next step much easier. How do we handle this exactly? If we get an image with ndim>2 and order='xy' should we raise an error or warning or just silently ignore the option?
|
|
||
| Returns | ||
| ------- | ||
| Axx : ndarray | ||
| Arr : ndarray |
There was a problem hiding this comment.
This return value is no longer correct for ndim > 2, is it?
There was a problem hiding this comment.
Let's leave this one to the follow-up PR adding tests and examples for ndim > 2 support
|
@emmanuelle why is this version not 3D? |
@jni the docstring suggests it's 2D with only Arc, etc. The code is indeed compatible with 3D but I think Corey wanted first to get the notations right and then tackle 3D in another PR. |
|
That's right. Switching to row/col notation is almost enough to support higher dimensions, but we also need to generalize |
|
🎉 Thanks @coreysharris! |
Description
This PR fixes the notation discrepancy in
skimage.feature.structure_tensorby changing from x/y-notation to row/col-notation as noted in #4831. It also begins preparations for the 3D structure tensor work done in #2987.Checklist
[x] Docstrings for all functions
[ ] Gallery example in
./doc/examples(new features only)[ ] Benchmark in
./benchmarks, if your changes aren't covered by anexisting benchmark
[x] Unit tests
[x] Clean style in the spirit of PEP8
For reviewers
[ ] Check that the PR title is short, concise, and will make sense 1 year
later.
[ ] Check that new functions are imported in corresponding
__init__.py.[ ] Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.