Skip to content

Jupyter Utilities#1797

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

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

Conversation

@ericspod

Copy link
Copy Markdown
Member

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.

Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
@ericspod

Copy link
Copy Markdown
Member Author

/black

ericspod and others added 2 commits March 17, 2021 20:25
Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
@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
Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
@ericspod ericspod requested a review from wyli March 19, 2021 00:29
Comment thread 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

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()


def __init__(
self,
loss_transform: Callable = _get_loss_from_output,

Copy link
Copy Markdown
Contributor

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),

return fig, axes


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

Copy link
Copy Markdown
Contributor

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

rijobro commented Mar 19, 2021

Copy link
Copy Markdown
Contributor

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

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

Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
@ericspod ericspod enabled auto-merge (squash) March 19, 2021 19:33
):
"""
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

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

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

wyli commented Mar 22, 2021

Copy link
Copy Markdown
Contributor

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

wyli commented Mar 23, 2021

Copy link
Copy Markdown
Contributor

followup testing addressed by #1826

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.

4 participants