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

Cloning MultitaskWrapper with postfix parameters results in incorrect dictionary #2656

Closed
Buzzeitor30 opened this issue Jul 26, 2024 · 7 comments · Fixed by #2722
Closed
Assignees
Labels
bug / fix Something isn't working question Further information is requested v1.4.x

Comments

@Buzzeitor30
Copy link

Buzzeitor30 commented Jul 26, 2024

🐛 Bug

If you clone a MultitaskWrapper and make usage of the postfix parameter, input dictionary keys seem to expect to have the postfix included at the end. However, documentation indicates that the output dictionary should be modified. I.e., the one resulting after having computed the values

To Reproduce

Steps to reproduce the behavior...

Expand
from torchmetrics import MultitaskWrapper, F1Score
import torch
wrapper = MultitaskWrapper({"F1": F1Score(num_classes=2, average='macro', task="multiclass")})
wrapper2 = wrapper.clone(postfix="train")
preds = {"F1": torch.ones((5, 2))}
tgt = {"F1": torch.tensor([0, 1, 0, 1, 0], dtype=torch.long)}

#wrapper(preds, tgt) # This one works
wrapper2(preds, tgt) 

Expected behavior

Resulting dictionary should include {"F1train" : ....}. Nevertheless, it throws a KeyError

Environment

  • TorchMetrics version 1.4.0.post0

Additional context

@Buzzeitor30 Buzzeitor30 added bug / fix Something isn't working help wanted Extra attention is needed labels Jul 26, 2024
Copy link

Hi! thanks for your contribution!, great first issue!

@Borda Borda added the v1.4.x label Jul 29, 2024
@Borda Borda changed the title Cloning MultitaskWrapper with postfix parameters results in incorrect dictionary Cloning MultitaskWrapper with postfix parameters results in incorrect dictionary Aug 2, 2024
@MihaiBabiac
Copy link

This is also the case for the prefix parameter. This means that unfortunately most of the changes from #2330 aren't really usable ☹️ . I think that PR assumed that changes to the dictionary keys will affect the output, as in the case of MetricCollection, not the input.

@MihaiBabiac
Copy link

@SkafteNicki what do you think about adding self.prefix and self.postfix member variables to the MultitaskWrapper and modifying MultitaskWrapper.keys() and MultitaskWrapper.items() to prepend/append them to the returned keys? I think that would fix the issue

@Borda
Copy link
Member

Borda commented Aug 21, 2024

@MihaiBabiac thank you for your feedback, I have check the sample code and looked to the implementation and so far it seems to me all is as expected, ref to the docs:

Args:
    prefix: a string to append in front of the metric keys
    postfix: a string to append after the keys of the output dict.

@Buzzeitor30 Could you pls elaborate on why would you be adding pre/postfix to the "metric's dictionary" which is not present in the key?

@Borda Borda added question Further information is requested and removed help wanted Extra attention is needed labels Aug 21, 2024
@MihaiBabiac
Copy link

Hi @Borda, thanks for the reply.

I think a common scenario is to clone a metric collection or, in this case, the MultiTaskWrapper in order to have separate metric objects for training, test and validation, each of them using different names when logged to tensorboard. This works for the collection, but not for the wrapper.

The similarity to the MetricCollection is also what makes the prefix and postfix arguments confusing, because they have different implications. For the metric collection, they just affect the keys in the output dictionary, meaning that the clones can be applied to the same input data. But for the multitask wrapper they also make the wrapper extract different entries in the input dictionary, so trying to apply the clones to the same data is unlikely to work.

If it's unclear or would like to have an MRE, just let me know.

@SkafteNicki
Copy link
Member

Hi @MihaiBabiac, thanks for raising this issue.
You are completely right that it is confusing that MetricCollection and MultitaskWrapper behaves in a different way. I have created PR #2722 that should fix this, making the prefix and postfix arguments work similar to how they work in MetricCollection

@Buzzeitor30
Copy link
Author

Sorry for answering back so late. It is, indeed, as @MihaiBabiac points out
@Borda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working question Further information is requested v1.4.x
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants