Skip to content

3694 loss function as cumulative metric#5513

Merged
wyli merged 3 commits intoProject-MONAI:devfrom
wyli:3694-loss-as-metric
Nov 16, 2022
Merged

3694 loss function as cumulative metric#5513
wyli merged 3 commits intoProject-MONAI:devfrom
wyli:3694-loss-as-metric

Conversation

@wyli
Copy link
Copy Markdown
Contributor

@wyli wyli commented Nov 11, 2022 •
edited
Loading

Signed-off-by: Wenqi Li [email protected]

Fixes #3694

Description

adds a wrapper for loss function:

dice_loss = DiceLoss(include_background=True)
loss_metric = LossMetric(loss_fn=dice_loss)

so that loss_metric it can be used as a metric in:

MONAI/monai/handlers/ignite_metric.py

Line 32 in b030839

class IgniteMetric(Metric): # type: ignore[valid-type, misc] # due to optional_import

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.

@wyli wyli marked this pull request as ready for review November 11, 2022 16:27
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Nov 12, 2022

Hi @wyli ,

Users can use the ignite metric directly:
https://github.com/pytorch/ignite/blob/master/ignite/metrics/loss.py
Just like how we use the Accuracy of ignite.
CC @vfdev-5 .

Thanks.

@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Nov 12, 2022

@Nic-Ma yes I'm aware of that, the assumptions on loss_fn is stronger there: -- requiring both y_pred and y, and -- the output must be a single value and --must be used with an ignite engine...

In our case, previously I thought the cumulative average is good enough,

MONAI/monai/metrics/cumulative_average.py

Line 21 in 89684d6

class CumulativeAverage:

But it doesn't seem to work with IgniteMetric handler.

@myron myron self-requested a review November 16, 2022 00:42
myron
myron requested changes Nov 16, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@myron myron left a comment

Choose a reason for hiding this comment

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

assumptions on loss_fn is stronger there: -- requiring both y_pred and y, and -- the output must be a single value and --must be used with an ignite engine...

@wyli thanks for the PR , but I don't think this is what people are asking for in the issue #3694, they did not ask for Ignite support. (unless I misunderstand).

And currently we can currently use

ac = CumulativeAverage()
loss_value = someloss(x,y)
ac.append(ac)

this will work even if loss_value is an array/list/tensor (and removes nans). I'll clarify what people in the issue are actually asking, unless this PR addresses something else

@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Nov 16, 2022

Hi @Nic-Ma do you have any further comments here?

Nic-Ma
Nic-Ma reviewed Nov 16, 2022
View reviewed changes
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Nov 16, 2022

@Nic-Ma yes I'm aware of that, the assumptions on loss_fn is stronger there: -- requiring both y_pred and y, and -- the output must be a single value and --must be used with an ignite engine...

In our case, previously I thought the cumulative average is good enough,

MONAI/monai/metrics/cumulative_average.py

Line 21 in 89684d6

class CumulativeAverage:

But it doesn't seem to work with IgniteMetric handler.

OK, that makes sense.

Thanks.

wyli added 3 commits November 16, 2022 17:59
@wyli
fixes Project-MONAI#3694
211d47f 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
docstrings
7b09318 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
update based on comments
ac72a8a 
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli force-pushed the 3694-loss-as-metric branch from a08e06a to ac72a8a Compare November 16, 2022 18:00
@myron
Copy link
Copy Markdown
Collaborator

myron commented Nov 16, 2022

Hi @myron, as I mentioned in the description of this PR, it's about using the loss value within monai.handlers.IgniteMetric. CumulativeAverage is not compatible within that class. If you are not sure about the PR, we can discuss, but please don't block it using your admin permission.

thank you for the reply. In the PR description on the top , it says that it fixes #3694
In my understanding it does not fix that issue, since people were asking for something else. Hence, I left the review and requested the change. Do you prefer me not leaving reviews at all or not requesting changes? ps: I changed the review status to approve.

@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Nov 16, 2022

/build

@wyli wyli enabled auto-merge (squash) November 16, 2022 20:27
@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Nov 16, 2022

/build

@wyli wyli merged commit bb3ecf6 into Project-MONAI:dev Nov 16, 2022
bhashemian pushed a commit to bhashemian/MONAI that referenced this pull request Nov 23, 2022
@wyli @bhashemian
3694 loss function as cumulative metric (Project-MONAI#5513)
fe595e1 
Signed-off-by: Wenqi Li <[email protected]>

Fixes Project-MONAI#3694

### Description
adds a wrapper for loss function:
```py
dice_loss = DiceLoss(include_background=True)
loss_metric = LossMetric(loss_fn=dice_loss)
```
so that `loss_metric` it can be used as a metric in:

https://github.com/Project-MONAI/MONAI/blob/b030839e98e6a00c1ab0e53545f60b62dc4da4a4/monai/handlers/ignite_metric.py#L32

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] 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).
- [x] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [x] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [x] In-line docstrings updated.
- [x] Documentation updated, tested `make html` command in the `docs/`
folder.

Signed-off-by: Wenqi Li <[email protected]>
Signed-off-by: Behrooz <[email protected]>
@wyli wyli deleted the 3694-loss-as-metric branch December 13, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nic-Ma Nic-Ma Nic-Ma approved these changes

@myron myron myron approved these changes

@rijobro rijobro Awaiting requested review from rijobro

@ericspod ericspod Awaiting requested review from ericspod ericspod is a code owner

@KumoLiu KumoLiu Awaiting requested review from KumoLiu KumoLiu is a code owner

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Generalize Metric to any loss function

3 participants

@wyli @Nic-Ma @myron