-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
thanks for the PR! for the two concerns:
|
Thanks for the great feedback! I'll get to it ASAP. |
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 |
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 :) |
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. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…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>
for more information, see https://pre-commit.ci
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
for more information, see https://pre-commit.ci
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>
Hello, for now I've removed the octave dependency, now the code only uses scipy solver. For an image with 118677 pixels (around 350 x 350), using i7-13700K with 10 runs each:
|
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
/build |
There was a problem hiding this 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.
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
Signed-off-by: Vahit Bugra YESILKAYNAK <bugrayesilkaynak@gmail.com>
/build |
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
./runtests.sh -f -u --net --coverage
../runtests.sh --quick --unittests --disttests
.make html
command in thedocs/
folder.