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

trainers.segmentation JaccardLoss receiving num_classes, should be a List[int]? #1896

Closed
robmarkcole opened this issue Feb 21, 2024 · 8 comments · Fixed by #1898
Closed

trainers.segmentation JaccardLoss receiving num_classes, should be a List[int]? #1896

robmarkcole opened this issue Feb 21, 2024 · 8 comments · Fixed by #1898

Comments

@robmarkcole
Copy link
Contributor

robmarkcole commented Feb 21, 2024

Description

From the trainer:

        elif loss == "jaccard":
            self.criterion = smp.losses.JaccardLoss(
                mode="multiclass", classes=self.hparams["num_classes"]
            )

num_classes is an int, Yet the docs for JaccardLoss state this is expected to be a list of int, presumably the indices of the output channels to use in the calc. Checking if this is a bug

Steps to reproduce

NA

Version

main

@calebrob6
Copy link
Member

I verified the behavior of JaccardLoss here https://gist.github.com/calebrob6/658edaa59c68f0c0a510f8d9d7a41458.

Currently our code will always use the average jaccard over all classes regardless of what self.hparams["num_classes"] is.

We definitely should fix this.

@isaaccorley ignore_index support might be a cool feature for the torchseg fork.

@robmarkcole
Copy link
Contributor Author

robmarkcole commented Feb 22, 2024

I suggest this requires a new arg of classes_in_loss which is a List[int]. This should pop off the ignore_index

@calebrob6
Copy link
Member

Have you run into scenarios in which you would want to ignore multiple classes? If not, I think we can just pop ignore_index from the entire class list if it is given.

@robmarkcole
Copy link
Contributor Author

I've not really used JaccardLoss, and commonly use nn.CrossEntropyLoss where the weights parameter has been very effective for imbalanced datasets. If I understand the use of classes in JaccardLoss it has a simiar purpose, but is binary on/off per class, rather than a weighting

Coming back to your question, for multiclass datasets where I might only care about 1 or 2 classes, I can imagine ignoring multiple classes

@calebrob6
Copy link
Member

+1 for fiddling with weights in CrossEntropyLoss.

Yep, I can imagine a few scenarios too (e.g. dropping loss over noisy classes). I would usually do this by simply remapping all classes I didn't care about to a nodata mask value, then setting ignore_index to that value, but this is more elegant. My concern now would be implementing this functionality for the other loss functions.

@adamjstewart adamjstewart added this to the 0.5.2 milestone Feb 25, 2024
@isaaccorley
Copy link
Collaborator

TorchSeg now supports both classes and ignore_index in DiceLoss, JaccardLoss, and FocalLoss.

@robmarkcole
Copy link
Contributor Author

@isaaccorley
Copy link
Collaborator

isaaccorley commented Feb 27, 2024

@robmarkcole it's merged into main now. I'll make another pre-release on pypi today.

@adamjstewart adamjstewart modified the milestones: 0.5.2, 0.6.0 Feb 29, 2024
@adamjstewart adamjstewart removed this from the 0.6.0 milestone Aug 29, 2024
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 a pull request may close this issue.

4 participants