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

MedicalNetPerceptualSimilarity: Add multi-channel #7568

Conversation

SomeUserName1
Copy link
Contributor

Fixes #7567 .

Description

MedicalNetPerceptualSimilarity: Add multi-channel support for 3Dvolumes.
The current version of the code in the dev branch already largely supports that besides the following:
medicalnet_* require inputs to have a single channel.
This PR passes the multi-channel volume channel-wise to the networks and concatenates the resulting feature vectors.
The existing code takes care of averaging over channels and spatially.

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.

    support for 3Dvolumes. The current version of the code in the dev
    branch already largely supports that besides the following:
    medicalnet_* require inputs to have a single channel. This PR passes
    the multi-channel volume channel-wise to the networks and
    concatenates the resulting feature vectors. The existing code takes
    care of averaging over channels and spatially.

Signed-off-by: Fabian Klopfer <[email protected]>
@KumoLiu
Copy link
Contributor

KumoLiu commented Mar 23, 2024

The implementation appears to be done correctly and it successfully introduces multi-channel input support for PerceptualSimilarity. It effectively allows each channel to be processed independently, which is a substantial addition.
However, I'd like to suggest the consideration of a specific use-case scenario. The new implementation seems to be most useful under situations where independent channel processing is beneficial. Conversely, in the final computation stage, results.sum(dim=1, keepdim=True) consolidates the multi-channel perception into a single scalar evaluation. This approach works well under single modality situations.
For cases involving multi-modal inputs, it might be worth considering the separate evaluation of each modality. As a suggestion, perhaps we could add an optional flag that switches between combined evaluation and individual modality evaluation. This could potentially enhance the generalization capabilities of the implementation and provide users the flexibility to choose based on their specific use case dynamics.

@SomeUserName1
Copy link
Contributor Author

Will adjust accordingly on monday, such that the return types are scalar or 1D array/tensor respectively.

@SomeUserName1
Copy link
Contributor Author

SomeUserName1 commented Mar 25, 2024

I tried to adapt it for RadNet and ResNet too, however as they expect 3 channel input and always output 2048 features, it's non trivial to treat the images channel-wise here.
At least i can not think of a trivial way to disentangle the effect that each of the 3 channels has on the 2048 features.

So I'd stick to MedicalNet for a channel-wise perceptual loss.

Will squash the commits as soon as LGTY

@SomeUserName1
Copy link
Contributor Author

@KumoLiu only the blossom-ci check hasn't passed. Can we merge?

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 quick update, few comments inline.

monai/losses/perceptual.py Outdated Show resolved Hide resolved
monai/losses/perceptual.py Outdated Show resolved Hide resolved
monai/losses/perceptual.py Outdated Show resolved Hide resolved
monai/losses/perceptual.py Outdated Show resolved Hide resolved
monai/losses/perceptual.py Outdated Show resolved Hide resolved
monai/losses/perceptual.py Outdated Show resolved Hide resolved
monai/losses/perceptual.py Outdated Show resolved Hide resolved
@SomeUserName1 SomeUserName1 requested a review from KumoLiu April 3, 2024 18:23
tests/test_perceptual_loss.py Outdated Show resolved Hide resolved
monai/losses/perceptual.py Outdated Show resolved Hide resolved
tags Outdated Show resolved Hide resolved
@KumoLiu
Copy link
Contributor

KumoLiu commented Apr 10, 2024

Hi @SomeUserName1, I noticed that you add me review the PR again.

In case you didn't noticed the comment here. Looks a little confuse to me here. Could you please help explain it more?
#7568 (comment)
Thanks.

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 quick update, overall looks good to me.
Only a small comment, will merge this ASAP.

monai/losses/perceptual.py Outdated Show resolved Hide resolved
@KumoLiu
Copy link
Contributor

KumoLiu commented Apr 19, 2024

/build

@KumoLiu KumoLiu enabled auto-merge (squash) April 19, 2024 13:30
@KumoLiu KumoLiu merged commit 03a5fa6 into Project-MONAI:dev Apr 19, 2024
28 checks passed
@SomeUserName1 SomeUserName1 deleted the 7567-multi-channel-medicalnetperceptualsimilarity branch April 19, 2024 16:13
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.

Perceptual Loss with mednet 3D multiple channels
2 participants