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

fix implementation mistakes and add conjugate gradients solver #7876

Merged
merged 23 commits into from
Jun 28, 2024

Conversation

MrGranddy
Copy link
Contributor

@MrGranddy MrGranddy commented Jun 25, 2024

Fixes #6767

Description

Fixed two minor implementation differences between the official MATLAB code and the MONAI implementation, to confirm also added a new test case where two images and their confidence maps calculated by the official code is added to tests and the MONAI implementation results are checked against there results created by the official code.

Also fixing the issue: added the conjugate gradients solver option. Now the users can utilize it to run the algorithm faster with a trade-off of accuracy of the end result, a range of speed-ups can be achieved with little to no quality loss by tweaking the parameters, the optimal parameters between quality and speed in my experience is set as the default parameters, namely 'cg_tol' and 'cg_maxiter'.

For the CG solver installing PyAMG (https://github.com/pyamg/pyamg) is a requirement, this is because we use it to generate a preconditioner, without it CG does not provide any speed-ups, even slows down the algorithm. This part can be changed if the requirement is not ideal, yet this was the best solution as far as my knowledge goes.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • 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.

@MrGranddy MrGranddy force-pushed the dev branch 2 times, most recently from 7b6f37f to 6cc85f5 Compare June 25, 2024 02:18
MrGranddy and others added 11 commits June 25, 2024 02:21
now these two fixes are made, when the algorithm is run using a deterministic matrix solver, it is exactly equal to the official implementation

Signed-off-by: MrGranddy <[email protected]>
This reverts commit 9b68188.

Signed-off-by: MrGranddy <[email protected]>
now these two fixes are made, when the algorithm is run using a deterministic matrix solver, it is exactly equal to the official implementation

Signed-off-by: MrGranddy <[email protected]>
Signed-off-by: MrGranddy <[email protected]>
Copy link
Contributor

@KumoLiu KumoLiu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Overall looks good to me, several minor comments inline.

@ericspod and @Nic-Ma, do you have any concern to include this third package pyamg?

monai/data/ultrasound_confidence_map.py Outdated Show resolved Hide resolved
requirements-dev.txt Outdated Show resolved Hide resolved
tests/test_ultrasound_confidence_map_transform.py Outdated Show resolved Hide resolved
monai/transforms/intensity/array.py Outdated Show resolved Hide resolved
@KumoLiu
Copy link
Contributor

KumoLiu commented Jun 25, 2024

Could you please also take a look at the CI error: https://github.com/Project-MONAI/MONAI/actions/runs/9655433015/job/26631317948?pr=7876#step:13:19809?

MrGranddy and others added 5 commits June 25, 2024 21:08
Co-authored-by: Eric Kerfoot <[email protected]>
Signed-off-by: Vahit Buğra YEŞİLKAYNAK <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Vahit Buğra YEŞİLKAYNAK <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Vahit Buğra YEŞİLKAYNAK <[email protected]>
mention 'cg_tol' and 'cg_maxiter' will only be used when 'use_cg' is set to 'True'

Signed-off-by: MrGranddy <[email protected]>
using 'tol' is deprecated and in the future this will need to be changed to rtol again, yet for now 'rtol' is not supported bny some scipy versions

Signed-off-by: MrGranddy <[email protected]>
Signed-off-by: MrGranddy <[email protected]>
@ericspod
Copy link
Member

The current twine issue appears related to a new version of a dependent package: pypa/twine#977 (comment) We could add a value in one of our config files to include the missing key or fix the version of importlib_metadata to be less than 8.0.

@KumoLiu
Copy link
Contributor

KumoLiu commented Jun 26, 2024

The current twine issue appears related to a new version of a dependent package: pypa/twine#977 (comment) We could add a value in one of our config files to include the missing key or fix the version of importlib_metadata to be less than 8.0.

twine==5.1.0 has been yanked: https://pypi.org/project/twine/#history.
I just rerun the job, it works well now.

@KumoLiu
Copy link
Contributor

KumoLiu commented Jun 27, 2024

Hi @ericspod, do you have any more comments on this one?

Copy link
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

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

Looks good now.

@ericspod ericspod enabled auto-merge (squash) June 27, 2024 20:19
@KumoLiu
Copy link
Contributor

KumoLiu commented Jun 28, 2024

/build

@ericspod ericspod merged commit 06cbd70 into Project-MONAI:dev Jun 28, 2024
28 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.

faster solver for computing confidence maps
3 participants