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.

Sign up for GitHub

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

Plot multiple 2D images as cards in the same tag #7725

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Plot multiple 2D images as cards in the same tag #7725

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion monai/visualize/img2tensorboard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ def plot_2d_or_3d_image(
dataformats = "HW"
for j, d2 in enumerate(d[:max_channels]):
d2 = rescale_array(d2, 0, 1)
writer.add_image(f"{tag}_{dataformats}_{j}", d2, step, dataformats=dataformats)
writer.add_image(f"{tag}_{dataformats}/{j}", d2, step, dataformats=dataformats)
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to preserve existing behaviour for users, this change should be controlled by a flag passed in as an argument to plot_2d_or_3d_image. with the default being the existing behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current behavior is inconsistent between different image types. Currently, multiple 3D images are plotted in the same tag while multiple 2D images are plotted in different tags. I would think consistency would be preferred over preserving the current behavior? Also, the flag should probably control all image sizes and not just 2D?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @MattTheCuber,
I agree that it is a little inconsistent in its current form, but preserving current behaviour is always preferred and we always like to warn users in advance if default behaviour is changing. If we were to change it without giving users the ability to control the behaviour it makes comparisons between runs done before and after the change very hard.

We would instead do the following:

  1. introduce the flag to switch between current behaviour and the suggested change
  2. schedule a change to the default setting for that flag for a future release and add suitable documentation so that users have time to change scripts if they need to do so

As for the 2d vs 3d discrepancy, I think there might be a broader question as to whether this method is sufficiently flexible and the best way of addressing it. We have a core development meeting today; I'll raise the issue and see what folks have to say, then come back to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @MattTheCuber, we can simply add a flag for this multi-channel 2d image plt. Thanks.

Copy link
Contributor

@atbenmurray atbenmurray May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@KumoLiu I was thinking that if we looked a bit more of a refactor we might want to give the caller more flexibility in how their data is displayed in tensorboard:

  • 2d data
    • image
  • 3d data
    • image ((rgb)HW)
    • image with '_' tag (CHW)
    • image with '/' tag (CHW)
    • video (DHW) / (THW)
    • animated_gif (DHW) / (THW)
  • 4d data
    • video (rgb(DHW)) / (rgb(THW))
    • animated_gif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, refactoring this would be nice. As I said previously, the current behavior is quite disjoint.

return

if d.ndim >= 4:
Expand Down
Loading