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

Add ultrasound confidence map to transforms #6709

Merged
merged 28 commits into from
Jul 23, 2023
Merged

Conversation

MrGranddy
Copy link
Contributor

@MrGranddy MrGranddy commented Jul 12, 2023

Description

This transform uses the method introduced by Karamalis et al. in https://doi.org/10.1016/j.media.2012.07.005 to
compute confidence maps on ultrasound images.

Possible Problems

  • I am not entirely sure if the "transforms" section is the right place for this method but I found it the most suitable
    since it is not "deep learning" and it is "pre-processing" in a way.

  • Current version of the implementation requires GNU Octave to be installed and defined in the path. This is an odd
    dependency, I am aware of that, yet using SciPy does not provide satisfactory results in terms of speed. If this kind of
    dependency is not suitable, I also have a pure SciPy implementation, yet it runs about x15 slower, and it is slow to work
    in real-time, I am open to any feedback.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@wyli
Copy link
Contributor

wyli commented Jul 12, 2023

thanks for the PR! for the two concerns:

  • perhaps the relevant functions could be moved to monai.data and only leave a thin wrapper in monai.transforms, so that the users can access the core functions directly without having to understand monai.transforms APIs.
  • I think it's fine to currently depend on GNU Octave, however we won't ship/redistribute Octave, so would be great to provide some instructions in the docstring about how to ensure necessary environment to run the components. or optionally, the component could automatically detect octave/scipy and run with the corresponding implementations. Including the scipy implementation will also make the unit testing easier because it's available in the testing environments.

@wyli wyli requested review from ericspod, Nic-Ma and KumoLiu July 12, 2023 11:45
@MrGranddy
Copy link
Contributor Author

thanks for the PR! for the two concerns:

  • perhaps the relevant functions could be moved to monai.data and only leave a thin wrapper in monai.transforms, so that the users can access the core functions directly without having to understand monai.transforms APIs.
  • I think it's fine to currently depend on GNU Octave, however we won't ship/redistribute Octave, so would be great to provide some instructions in the docstring about how to ensure necessary environment to run the components. or optionally, the component could automatically detect octave/scipy and run with the corresponding implementations. Including the scipy implementation will also make the unit testing easier because it's available in the testing environments.

Thanks for the great feedback! I'll get to it ASAP.

@MrGranddy MrGranddy closed this Jul 12, 2023
@ericspod
Copy link
Member

ericspod commented Jul 12, 2023

Hi @MrGranddy, looks like a good addition and moving function elsewhere and using the transform as a wrapper is a good idea. However I don't think keeping Octave here is a good idea, it's a large dependency and probably isn't very fast. I'd also suggest not relying on scipy so that people can avoid having to install it as well. The fewer dependencies we have the fewer issues with restricted environments we'll encounter.

In general I think your code can be converted to be all Pytorch and avoid using Numpy/Octave entirely. The mldivide function I believe can be done with torch.linalg.solve (or at least is close enough), we have a hilbert transform layer already, resample layers to avoid using OpenCV, and otherwise use Pytorch equivalents in places. The use of csc_matrix I think we can just get around with a different structure. Using Pytorch only (or switch to a from numpy is absolutely needed) would be much faster and more coherent, we could even consider doing this calculation on GPU in that case. I would suggest porting your algorithm to Pytorch then for your next submission.

@MrGranddy
Copy link
Contributor Author

Hi @MrGranddy, looks like a good addition and moving function elsewhere and using the transform as a wrapper is a good idea. However I don't think keeping Octave here is a good idea, it's a large dependency and probably isn't very fast. I'd also suggest not relying on scipy so that people can avoid having to install it as well. The fewer dependencies we have the fewer issues with restricted environments we'll encounter.

In general I think your code can be converted to be all Pytorch and avoid using Numpy/Octave entirely. The mldivide function I believe can be done with torch.linalg.solve (or at least is close enough), we have a hilbert transform layer already, resample layers to avoid using OpenCV, and otherwise use Pytorch equivalents in places. The use of csc_matrix I think we can just get around with a different structure. Using Pytorch only (or switch to a from numpy is absolutely needed) would be much faster and more coherent, we could even consider doing this calculation on GPU in that case. I would suggest porting your algorithm to Pytorch then for your next submission.

Hello, the original implementation actually does exactly what you describe, but it works really slow due to the nature of the algorithm, SciPy sparse solvers are not as fast as the Octave one, PyTorch is even slower. So the main problem here is that the problem solves for a sparse matrix, and that matrix gets really big really fast since it solves for the adjacency matrix of an image. NxN image means N^2 x N^2 matrix. PyTorch sparse library is very underdeveloped as far as I know, and SciPy sparse solver is 15x times slower compared to the Octave solver, at least on Windows, there is the possibility Linux binaries are better but I couldn't test it on a Linux machine. I also tried stochastic solvers like conjugate gradients, they are fast but they do not produce good results.

I also closed the PR by accident, can you please open it again :) Thank you. Looking forward to your feedback as always :)

@wyli wyli reopened this Jul 12, 2023
@ericspod
Copy link
Member

Hi @MrGranddy I see the problem then a little better. The issue though is the process for installing Octave and oct2py is more complex than other things we rely on, requiring a separate install and a correctly configured PATH variable. This is also an issue for our CICD system since we do need to test this transform. The library you're using, oct2py, also seems like a rather small project whose long term support is not assured. I'd still recommend we look into some other solver (something wrapping eigen?) to avoid this dependency.

MrGranddy and others added 18 commits July 19, 2023 03:47
…gmail.com>

I, Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>, hereby add my Signed-off-by to this commit: 11a1399
I, Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>, hereby add my Signed-off-by to this commit: 14b7af0
I, Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>, hereby add my Signed-off-by to this commit: c4ab0b7

Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
@MrGranddy
Copy link
Contributor Author

MrGranddy commented Jul 19, 2023

Hi @MrGranddy I see the problem then a little better. The issue though is the process for installing Octave and oct2py is more complex than other things we rely on, requiring a separate install and a correctly configured PATH variable. This is also an issue for our CICD system since we do need to test this transform. The library you're using, oct2py, also seems like a rather small project whose long term support is not assured. I'd still recommend we look into some other solver (something wrapping eigen?) to avoid this dependency.

Hello, for now I've removed the octave dependency, now the code only uses scipy solver.
As far as my knowledge goes, there is no decomposition or iterative method that would make
the algorithm faster while preserving the results. I am also sharing some metrics for the comparison
of octave and scipy. I believe SciPy is not all bad, considering they also would optimize their sparse solver
over time.

image

For an image with 118677 pixels (around 350 x 350), using i7-13700K with 10 runs each:

Scipy mean time: 5.763958525657654 +- 0.2072540601999276
Octave mean time: 0.7772286891937256 +- 0.018855939984020915

MrGranddy and others added 3 commits July 19, 2023 06:33
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
@wyli
Copy link
Contributor

wyli commented Jul 19, 2023

/build

Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks, it looks good to me, could you please update the docstring to describe the expected shape of the main input arrays such as img, labels? after this PR I think it would be great to create a feature request to enhance efficiency beyond the scipy implementation.

MrGranddy and others added 4 commits July 24, 2023 00:15
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
@wyli
Copy link
Contributor

wyli commented Jul 23, 2023

/build

@wyli wyli enabled auto-merge (squash) July 23, 2023 22:47
@wyli wyli merged commit 3410794 into Project-MONAI:dev Jul 23, 2023
26 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants