Skip to content

Jupyter Utilities#1797

Merged
ericspod merged 18 commits intoProject-MONAI:masterfrom
ericspod:master
Mar 19, 2021
Merged

Jupyter Utilities#1797
ericspod merged 18 commits intoProject-MONAI:masterfrom
ericspod:master

Conversation

@ericspod
Copy link
Copy Markdown
Member

@ericspod ericspod commented Mar 17, 2021

Description

This includes some utilities to help use MONAI in Jupyter. I will have an example notebook shortly for the tutorials repo. One issue is that the type checking was difficult to fully include and make work with Matplotlib so some values are untyped, and optional_import cause circular dependency issues in the utility file.

Status

Ready

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.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@ericspod
Copy link
Copy Markdown
Member Author

ericspod commented Mar 17, 2021

/black

@ericspod ericspod requested review from Nic-Ma and rijobro March 17, 2021 20:26
ericspod and others added 10 commits March 17, 2021 20:29
@ericspod ericspod requested a review from wyli March 19, 2021 00:29
rijobro
rijobro reviewed Mar 19, 2021
View reviewed changes
monai/handlers/metric_logger.py Outdated
Comment on lines +26 to +27
def _get_loss_from_output(output):
return output["loss"].item()
Copy link
Copy Markdown
Contributor

@rijobro rijobro Mar 19, 2021

Choose a reason for hiding this comment

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

Could be:

Suggested change
def _get_loss_from_output(output):
return output["loss"].item()
def _get_loss_from_output(output, loss_key = "loss"):
return output[loss_key].item()

rijobro
rijobro reviewed Mar 19, 2021
View reviewed changes
monai/handlers/metric_logger.py

def __init__(
self,
loss_transform: Callable = _get_loss_from_output,
Copy link
Copy Markdown
Contributor

@rijobro rijobro Mar 19, 2021

Choose a reason for hiding this comment

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

Might be overkill but could be:

Suggested change
loss_transform: Callable = _get_loss_from_output,
loss_transform: Callable = partial(_get_loss_from_output, loss_key=MetricLoggerKeys.LOSS),

rijobro
rijobro reviewed Mar 19, 2021
View reviewed changes
monai/utils/jupyter_utils.py
return fig, axes


def _get_loss_from_output(output: Union[Dict[str, torch.Tensor], torch.Tensor]) -> float:
Copy link
Copy Markdown
Contributor

@rijobro rijobro Mar 19, 2021

Choose a reason for hiding this comment

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

Could you call the other implementation to reduce duplication?

@rijobro
Copy link
Copy Markdown
Contributor

rijobro commented Mar 19, 2021

Looks good. I really like plotting losses with live updates, which makes my tutorials more bloated than necessary. Would be great if that functionality could be incorporated somehow (see use of plot_range in https://app.reviewnb.com/Project-MONAI/tutorials/pull/129/).

@ericspod
Copy link
Copy Markdown
Member Author

ericspod commented Mar 19, 2021

plot_range and the other plotting functions should be consolidated in one place, I have other plot functions to add later on.

@ericspod
Update
cda427d 
Signed-off-by: Eric Kerfoot <[email protected]>
@ericspod ericspod enabled auto-merge (squash) March 19, 2021 19:33
wyli
wyli reviewed Mar 19, 2021
View reviewed changes
monai/utils/jupyter_utils.py
):
"""
Plot metrics on a single graph with running averages plotted for selected keys. The values in `graphmap`
should be lists of (timepoint, value) pairs as stored in MetricLogger objects.
Copy link
Copy Markdown
Contributor

@wyli wyli Mar 19, 2021

Choose a reason for hiding this comment

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

could you create a few tests for the plotting method and MetricLogger? came across this util not sure how reliable it is https://github.com/matplotlib/matplotlib/blob/6fe34e72d2b7daf8586a08a8611cc73b3c0ad60c/lib/matplotlib/testing/decorators.py#L393

Copy link
Copy Markdown
Contributor

@wyli wyli Mar 20, 2021

Choose a reason for hiding this comment

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

I forgot to request changes for unit tests, could you please have a follow-up PR, thank you! @ericspod

@ericspod ericspod merged commit fdf26fb into Project-MONAI:master Mar 19, 2021
@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 22, 2021 •
edited
Loading

without tests it's difficult for the other maintainers to understand the intended behaviour and ensure the quality...
as we have a tight schedule for v0.5, If we don't have proper tests for this PR within this week, I'll try to exclude it from the upcoming release to avoid any issues

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 23, 2021

followup testing addressed by #1826

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli left review comments

@rijobro rijobro rijobro approved these changes

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ericspod @rijobro @wyli @Nic-Ma