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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CriticalSuccessIndex (CSI) metric #2257

Merged
merged 22 commits into from
Dec 21, 2023

Conversation

miskfi
Copy link
Contributor

@miskfi miskfi commented Dec 2, 2023

What does this PR do?

Fixes #1509

Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
PR review

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 馃檭

Notes

  • I didn't write any unit tests yet, as I didn't know what implementation could be used as a reference (torchmetrics and scikit-learn have Jaccard index, which is essentially the same calculation, but it requires different inputs than CSI)
  • I didn't update the docs as I had some trouble installing lai-sphinx-theme
  • The metric could be expanded to provide other skill scores as they mostly differ only in the compute() method. But for now I only implemented the CSI.
  • This is my first PR here, so I apologize in advance for any mistakes against your code style or standards.

@SkafteNicki SkafteNicki added this to the v1.3.0 milestone Dec 3, 2023
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Dec 3, 2023
@SkafteNicki
Copy link
Member

@miskfi super work already :)
Ofcause the testing is missing, do you know of any third party implementation that it is possible to compare against?

@miskfi
Copy link
Contributor Author

miskfi commented Dec 4, 2023

@SkafteNicki Thanks, I added some unit tests against jaccard_score from scikit-learn (it requires thresholding of the input tensors). There is also direct implementation of ThreatScore in tensorflow_model_analysis but I didn't understand how to make it work on raw data without some tf model.

One thing I came across when writing the tests was the keep_sequence_dim parameter I have. I noticed that the tester passed the tensors to the metric without the batch dimension. But my idea was that the parameter would represent an index of the dimension in the original tensor (with the batch dimension) so without the batch dimension this index was out of range. Maybe this whole parameter isn't a good idea, I just implemented it as it was useful for my use case.

@Borda
Copy link
Member

Borda commented Dec 4, 2023

LGTM over all with added tests; let,s target the failing tests...
RuntimeError: output with shape [] doesn't match the broadcast shape [2]

@Borda
Copy link
Member

Borda commented Dec 18, 2023

@miskfi mind checking:

UNEXPECTED EXCEPTION: TypeError('Subscripted generics cannot be used with class and instance checks')
Traceback (most recent call last):
  File "/Users/runner/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/doctest.py", line 1336, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest torchmetrics.image.csi.CriticalSuccessIndex[4]>", line 1, in <module>
  File "/Users/runner/work/torchmetrics/torchmetrics/src/torchmetrics/image/csi.py", line 66, in __init__
    if (not isinstance(keep_sequence_dim, Optional[int])) or (keep_sequence_dim and keep_sequence_dim < 0):
  File "/Users/runner/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/typing.py", line 769, in __instancecheck__
    return self.__subclasscheck__(type(obj))
  File "/Users/runner/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/typing.py", line 777, in __subclasscheck__
    raise TypeError("Subscripted generics cannot be used with"
TypeError: Subscripted generics cannot be used with class and instance checks

@SkafteNicki SkafteNicki self-assigned this Dec 20, 2023
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Merging #2257 (b5366ff) into master (20a1fd1) will decrease coverage by 47%.
Report is 1 commits behind head on master.
The diff coverage is 73%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2257     +/-   ##
========================================
- Coverage      87%     40%    -47%     
========================================
  Files         294     296      +2     
  Lines       16633   16709     +76     
========================================
- Hits        14442    6734   -7708     
- Misses       2191    9975   +7784     

@Borda Borda enabled auto-merge (squash) December 20, 2023 12:31
@mergify mergify bot added the ready label Dec 20, 2023
@Borda Borda merged commit b9fe394 into Lightning-AI:master Dec 21, 2023
59 of 60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation New metric ready topic: Image
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Critical Success Index to Torchmetrics
4 participants