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.

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

[Metrics] Confusion matrix class interface #4348

Merged
merged 32 commits into from Oct 30, 2020

Conversation

SkafteNicki
Copy link
Member

What does this PR do?

  • Adds back confusion matrix class interface
  • Unify with functional interface
  • Updated old test to new interface

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@SkafteNicki SkafteNicki added feature Is an improvement or enhancement Metrics labels Oct 25, 2020
@SkafteNicki SkafteNicki added this to the 1.1 milestone Oct 25, 2020
@SkafteNicki SkafteNicki merged commit e0b856c into Lightning-AI:master Oct 30, 2020
Metrics package automation moved this from in Progress to Done Oct 30, 2020
@SkafteNicki SkafteNicki deleted the metrics/confusion_matrix branch October 30, 2020 10:44
@ydcjeff ydcjeff modified the milestones: 1.1, 1.0.x Oct 30, 2020
@Vichoko
Copy link
Contributor

Vichoko commented Oct 30, 2020

In which ptl version this will be released?

@ydcjeff
Copy link
Contributor

ydcjeff commented Oct 30, 2020

In which ptl version this will be released?

it could be in coming point release on Tuesday.

@NumesSanguis
Copy link
Contributor

According to the release changelog, ConfusionMatrix should be in PL v1.0.5.
However, trying to import it, fails:

>>> from pytorch_lightning.metrics import ConfusionMatrix
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'ConfusionMatrix' from 'pytorch_lightning.metrics' (*path*/python3.8/site-packages/pytorch_lightning/metrics/__init__.py)

Colab: https://colab.research.google.com/drive/1QHFCBtGEv215DuLw6Suqi9A1WHiExlZR?usp=sharing

@ydcjeff
Copy link
Contributor

ydcjeff commented Nov 4, 2020

@NumesSanguis I think they cherry-picked the commits for 1.0.5 release so it didn't include in it. maybe in 1.1

@NumesSanguis
Copy link
Contributor

@ydcjeff Then the patch notes are wrong:

Added ConfusionMatrix class interface (#4348)
Added multiclass AUROC metric (#4236)

You can find this Metric also already in the docs:
https://pytorch-lightning.readthedocs.io/en/latest/metrics.html#confusionmatrix

@ydcjeff
Copy link
Contributor

ydcjeff commented Nov 4, 2020

Yep, we are in the transition of point release and minor release, we will edit the changelog

@s-rog
Copy link
Contributor

s-rog commented Nov 4, 2020

#4505

@NumesSanguis
Copy link
Contributor

I don't see an associated Feature Request, so I hope bug reports here are ok.

@SkafteNicki I tried this ConfusionMatrix using the master branch (pip install git+https://github.com/PytorchLightning/pytorch-lightning.git@master --upgrade), but there is an error when the last batch size does not match earlier batches:

     25                              threshold: float = 0.5) -> torch.Tensor:
     26     preds, target = _input_format_classification(preds, target, threshold)
---> 27     unique_mapping = (target.view(-1) * num_classes + preds.view(-1)).to(torch.long)
     28     bins = torch.bincount(unique_mapping, minlength=num_classes ** 2)
     29     confmat = bins.reshape(num_classes, num_classes)

RuntimeError: The size of tensor a (32) must match the size of tensor b (16) at non-singleton dimension 0

Colab: https://colab.research.google.com/drive/1QHFCBtGEv215DuLw6Suqi9A1WHiExlZR?usp=sharing

@NumesSanguis
Copy link
Contributor

@SkafteNicki There is also an issue (if you make sure the above batch size does not show up) when trying to log the Confusion Matrix with the v1.0.x logging approach:

self.confmat(output.softmax(dim=1), labels)
self.log(f"confmat/val", self.confmat)

Gives:

/usr/local/lib/python3.6/dist-packages/pytorch_lightning/trainer/logging.py in metrics_to_scalars(self, metrics)
     46         for k, v in metrics.items():
     47             if isinstance(v, torch.Tensor):
---> 48                 v = v.item()
     49 
     50             if isinstance(v, dict):

ValueError: only one element tensors can be converted to Python scalars

Colab: https://colab.research.google.com/drive/1GRi0VhRTbDGdj4D1HUMo49Pe7__tWe31?usp=sharing

@s-rog
Copy link
Contributor

s-rog commented Nov 4, 2020

@NumesSanguis Mind opening a separate issue?

@SkafteNicki
Copy link
Member Author

I don't see an associated Feature Request, so I hope bug reports here are ok.

@SkafteNicki I tried this ConfusionMatrix using the master branch (pip install git+https://github.com/PytorchLightning/pytorch-lightning.git@master --upgrade), but there is an error when the last batch size does not match earlier batches:

     25                              threshold: float = 0.5) -> torch.Tensor:
     26     preds, target = _input_format_classification(preds, target, threshold)
---> 27     unique_mapping = (target.view(-1) * num_classes + preds.view(-1)).to(torch.long)
     28     bins = torch.bincount(unique_mapping, minlength=num_classes ** 2)
     29     confmat = bins.reshape(num_classes, num_classes)

RuntimeError: The size of tensor a (32) must match the size of tensor b (16) at non-singleton dimension 0

Colab: https://colab.research.google.com/drive/1QHFCBtGEv215DuLw6Suqi9A1WHiExlZR?usp=sharing

This example does not reflect a real example. When you are creating artificial labels, when you do batch[1].sigmoid() you are creating a label tensor along the feature dimension, but should instead create it along the batch dimension like this i.e. batch[:,0].sigmoid().

@SkafteNicki There is also an issue (if you make sure the above batch size does not show up) when trying to log the Confusion Matrix with the v1.0.x logging approach:

self.confmat(output.softmax(dim=1), labels)
self.log(f"confmat/val", self.confmat)

Gives:

/usr/local/lib/python3.6/dist-packages/pytorch_lightning/trainer/logging.py in metrics_to_scalars(self, metrics)
     46         for k, v in metrics.items():
     47             if isinstance(v, torch.Tensor):
---> 48                 v = v.item()
     49 
     50             if isinstance(v, dict):

ValueError: only one element tensors can be converted to Python scalars

Colab: https://colab.research.google.com/drive/1GRi0VhRTbDGdj4D1HUMo49Pe7__tWe31?usp=sharing

self.log does not allow logging anything else than scalar tensors. This is a general limitation of self.log (not specific related to confusion matrix.

@Vichoko
Copy link
Contributor

Vichoko commented Nov 4, 2020

If there is no logging support through self.log, how we're supposed to aggregate the confusion matrix across batches and GPUs (on a multi-GPU scheme)?

@SkafteNicki
Copy link
Member Author

@Vichoko depending on what logger you are using, you can create the confusion matrix as a figure and log it. For example using the default tensorboard logger:

import matplotlib.pyplot as plt
...
def training_step(self, batch, batch_idx):
    ...
    step_confmat = self.metric(pred, target)
    fig = plt.figure(); plt.imshow(step_confmat)
    self.logger.experiment.add_figure('step_confmat', fig, global_step=self.global_step)

def training_epoch_end(self, outputs):
    epoch_confmat = self.metric.compute()
    fig = plt.figure(); plt.imshow(epoch_confmat)
    self.logger.experiment.add_figure('epoch_confmat', fig, global_step=self.global_step)

@NumesSanguis
Copy link
Contributor

This example does not reflect a real example. When you are creating artificial labels, when you do batch[1].sigmoid() you are creating a label tensor along the feature dimension, but should instead create it along the batch dimension like this i.e. batch[:,0].sigmoid().

Sorry, somehow I had my own classification problem in mind where I assumed that batch was a tuple of (inputs, labels), hence I used batch[1].


Thank you for the confusion matrix example, that was actually what I was searching for!

@NumesSanguis
Copy link
Contributor

I assume the x-axis (bottom) to represent the Predicted label, and the y-axis (left) to be the True label, but could this be added to the documentation?

@SkafteNicki
Copy link
Member Author

We follow sklearns way of representing confusion matrices (https://scikit-learn.org/stable/modules/generated/sklearn.metrics.confusion_matrix.html#sklearn.metrics.confusion_matrix) which indeed is predicted by x-axis/columns and true by y-axis/rows.
@NumesSanguis please feel free to open a PR with recommended doc update.

@Borda
Copy link
Member

Borda commented Nov 6, 2020

@SkafteNicki @NumesSanguis changelog shall be fixed in #4505

SeanNaren pushed a commit that referenced this pull request Nov 10, 2020
* docs + precision + recall + f_beta + refactor

Co-authored-by: Teddy Koker <teddy.koker@gmail.com>

* rebase

Co-authored-by: Teddy Koker <teddy.koker@gmail.com>

* fixes

Co-authored-by: Teddy Koker <teddy.koker@gmail.com>

* added missing file

* docs

* docs

* extra import

* add confusion matrix

* add to docs

* add test

* pep8 + isort

* update tests

* move util function

* unify functional and class

* add to init

* remove old implementation

* update tests

* pep8

* add duplicate

* fix doctest

* Update pytorch_lightning/metrics/classification/confusion_matrix.py

Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

* changelog

* bullet point args

* bullet docs

* bullet docs

Co-authored-by: ananyahjha93 <ananya@pytorchlightning.ai>
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: chaton <thomas@grid.ai>
Co-authored-by: Roger Shieh <55400948+s-rog@users.noreply.github.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

(cherry picked from commit e0b856c)
@Vichoko
Copy link
Contributor

Vichoko commented Nov 11, 2020

Changelog implied that this would be released on 1.0.6 but it wasn't :c. I should be worried about using this feature before 1.1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet