Skip to content

Unify visualisation#1381

Merged
wyli merged 20 commits intoProject-MONAI:masterfrom
rijobro:unify_visualisation
Jan 6, 2021
Merged

Unify visualisation#1381
wyli merged 20 commits intoProject-MONAI:masterfrom
rijobro:unify_visualisation

Conversation

@rijobro
Copy link
Copy Markdown
Contributor

@rijobro rijobro commented Dec 17, 2020

Unify visualisation classes.

Description

Beforehand, occlusion sensitivity was a function in metrics, whereas gradcam functionality was in classes inside the visualize module. Now, both can be called with similar calls:

from monai.networks.nets import densenet121
from monai.visualise import OcclusionSensitivity
from monai.visualise import CAM
model_2d = densenet121(spatial_dims=2, in_channels=1, out_channels=3)

cam = CAM(nn_module=model_2d, target_layers="class_layers.relu", fc_layers="class_layers.out")
result_cam = cam(x=torch.rand((1, 1, 48, 64)))

occ_sens = OcclusionSensitivity(nn_module=model_2d)
result_occ_sens = occ_sens(x=torch.rand((1, 1, 48, 64)), class_idx=None, b_box=[-1, -1, 2, 40, 1, 62])

We also had mixed American and British spelling, so changed all instances of visualize to visualise. Maybe I should have gone the other way, as it made lots of changes...

Status

Ready

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 --codeformat --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@rijobro rijobro mentioned this pull request Dec 17, 2020
Closed
@rijobro rijobro force-pushed the unify_visualisation branch from 3d2aa27 to 8878366 Compare December 17, 2020 17:04
@rijobro
unify visualisation
88abbe7 
Signed-off-by: Richard Brown <[email protected]>
@rijobro rijobro force-pushed the unify_visualisation branch from 8878366 to 88abbe7 Compare December 17, 2020 17:06
@wyli
Copy link
Copy Markdown
Contributor

wyli commented Dec 17, 2020 •
edited
Loading

I feel we should go for visualize... to be consistent with pytorch and numpy
thanks for raising this!

@rijobro
visualis* -> visualiz*
5082bdd 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
typos
f73e737 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
typo
20ec6fb 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Dec 17, 2020

ok, updated.

@rijobro rijobro mentioned this pull request Dec 17, 2020
Merged
6 tasks
@wyli
Copy link
Copy Markdown
Contributor

wyli commented Dec 18, 2020

thanks for the clean up! but I think it's too early to define a proper NetVisualizer API, we should keep the module simple for now, and it needs more discussions along with #528

@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Jan 4, 2021 •
edited
Loading

but I think it's too early to define a proper NetVisualizer API

I can remove it if you'd like but it's only a skeleton at the moment, with a constructor and __call__ method. Just so that when future visualisations get added, they are more likely to conform (avoid the difference we saw with OccSens and GradCAM).

Up to you, I'll delete the base class if you'd like, but I'm in favour of keeping it.

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Jan 4, 2021

but I think it's too early to define a proper NetVisualizer API

I can remove it if you'd like but it's only a skeleton at the moment, with a constructor and __call__ method. Just so that when future visualisations get added, they are more likely to conform (avoid the difference we saw with OccSens and GradCAM).

Up to you, I'll delete the base class if you'd like, but I'm in favour of keeping it.

I feel the benefit of having a NetVisualizer class is not very clear at the moment (and the interface is also not well documented), all the other changes look very nice. perhaps @Nic-Ma @ericspod could have more comments.

@rijobro
small changes
deef782 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Jan 4, 2021

Ok, I can remove it.

I also created a train_mode context manager (opposite of eval_mode) here. We can use this when calculating the gradient for GradCam, which should avoid the problem you saw here: Project-MONAI/tutorials#83 (comment).

@rijobro
occ sens add verbose option
05668c3 
Signed-off-by: Richard Brown <[email protected]>
@rijobro rijobro mentioned this pull request Jan 6, 2021
Merged
wyli
wyli approved these changes Jan 6, 2021
View reviewed changes
Copy link
Copy Markdown
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!

@wyli wyli merged commit a74fef9 into Project-MONAI:master Jan 6, 2021
@rijobro rijobro deleted the unify_visualisation branch January 6, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli approved these changes

@Nic-Ma Nic-Ma Awaiting requested review from Nic-Ma

@yiheng-wang-nv yiheng-wang-nv Awaiting requested review from yiheng-wang-nv

@ericspod ericspod Awaiting requested review from ericspod

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rijobro @wyli