8000 WIP: Adds 3D structure tensor capability by scott-trinkle · Pull Request #2987 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

WIP: Adds 3D structure tensor capability #2987

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

WIP: Adds 3D structure tensor capability #2987

wants to merge 7 commits into from

Conversation

scott-trinkle
Copy link

Description

Adds skimage.feature.structure_tensor_3D and skimage.feature.structure_tensor_3D_eig, which calculates the 3x3 structure tensor from a 3D image volume, and finds its eigenvalues and eigenvectors, respectively.

Checklist

References

Closes Issue #2972
Mathematical description available here

For reviewers

(Don't remove the checklist below.)

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

@pep8speaks
Copy link
pep8speaks commented Mar 18, 2018 •
edited
Loading

Hello @scott-trinkle! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on March 19, 2018 at 18:30 Hours UTC

@scott-trinkle scott-trinkle changed the title 3 d structure tensor Adds 3D structure tensor capability Mar 18, 2018
@scott-trinkle scott-trinkle changed the title Adds 3D structure tensor capability WIP: Adds 3D structure tensor capability Mar 19, 2018
@scott-trinkle
Copy link
Author

This is my first time contributing, I'm somewhat new at this - I can't figure out why the Travis CI builds keep failing. The tests are running successfully for me locally. Anyone see any glaring things I'm missing?

@jni
Copy link
Member
jni commented Mar 22, 2018

Hi @scott-trinkle!

Regarding your Travis failures, you can click through (under Details) to see whether the failure is related to your code or not. In your case it is: I guess you are not running doctests locally, because it is the doctest that is failing:
https://travis-ci.org/scikit-image/scikit-image/jobs/355506182#L4407

More broadly, thanks for your contribution, but I'm sad to say it needs a lot of refinement. I'd actually gotten started on this (thanks for your earlier issue report!) but was taking long because the "right" way, with proper deprecations, is quite tricky. Here are the goals I have for the structure tensor 3d code:

  1. Use plane, row, column coordinates instead of xyz, as described here: http://scikit-image.org/docs/dev/user_guide/numpy_images.html#coordinate-conventions
  2. Use a unified function (structure_tensor) for both 2D and 3D. See for example the code for hessian_matrix.
  3. Have consistent code between 2D and 3D.

To satisfy those goals, we need to deprecate the current 2D xy approach in favour of row/column (rc) coordinates. There is a "model" PR for converting Hessian matrix to 3D with unified code implemented here:

https://github.com/scikit-image/scikit-image/pull/2194/files

and a deprecation of the 2D xy code here:

https://github.com/scikit-image/scikit-image/pull/2327/files

Essentially this is the path that I want to take with the structure tensor. Feel up to it @scott-trinkle?? Sorry if this is a bit overwhelming, but I am very happy to help you through the process! We can even set up some time to do it "live" on gitter.im/scikit-image/scikit-image. Let me know if you're interested!

@scott-trinkle
Copy link
Author

Hi @jni,

Thanks so much for your feedback! I've been busy the past few weeks, but I now have some time to pick this up again, I want to give it a shot with your suggestions.

A few questions:

  1. Since I'll be basically discarding everything I've done up to this point, is it best to open a new branch/pull request?

  2. You are correct, I was not running the doctests locally. Looking closer at the "Building docs" section of the contribution instructions, it seems like I should be able to run the doctests with make doctests from the doc directory. Is that accurate?

I did a fresh clone/install using the developer instructions here. When I run make doctests from doc, I got the following error:

python /Users/scotttrinkle/anaconda/envs/ski/bin/sphinx-build -b doctest -d build/doctrees  -j 1 source build/doctest
Running Sphinx v1.7.2

Extension error:
Could not import extension sphinx_gallery.gen_gallery (exception: No module named 'sphinx_gallery')
make: *** [doctest] Error 2

So I ran pip install sphinx-gallery, and ran make doctest again after sphinx-gallery installed, and got the following error:

python /Users/scotttrinkle/anaconda/envs/ski/bin/sphinx-build -b doctest -d build/doctrees  -j 1 source build/doctest
Running Sphinx v1.7.2

Sphinx error:
Builder name doctest not registered or available through entry point
make: *** [doctest] Error 2

Any idea what could be going on? I see from the MakeFile that the doctest help section says "to run all doctests embedded in the documentation (if enabled)". Is there something I need to do to enable doctest?

Thanks again!

@jni
Copy link
Member
jni commented Apr 5, 2018

@scott-trinkle sigh unfortunately I don't. imho our test machinery is way too complicated/brittle. Certainly there should be no reason for sphinx to be involved in running doctests. I usually rely on manually reproducing the tests I am directly affecting, and then letting travis do the rest. @stefanv, any suggestions here?

As to question 1, I think you are right that the cleanest approach is to start a new PR. Just make sure to refer to this one from the new one.

@ghost
Copy link
ghost commented Feb 19, 2019

Maybe a better place to inquire: any progress on this in the last year? Thank you! L.

@emmanuelle emmanuelle mentioned this pull request Jul 8, 2020
Closed
3 tasks
Copy link
Member

It'd be great to finish this work, I created #4831 to discuss this and related issues with structure_tensor.

@coreysharris coreysharris mentioned this pull request Jul 11, 2020
Merged
@alexdesiqueira alexdesiqueira mentioned this pull request Jul 14, 2020
Closed
@stefanv stefanv closed this Feb 18, 2021
@stefanv stefanv deleted the branch scikit-image:master February 18, 2021 18:23
@mkcor mkcor mentioned this pull request Feb 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@scott-trinkle @pep8speaks @jni @emmanuelle @stefanv
0