Rename Serializer to Output and move to flash.core.data.io.output
#927
Conversation
ethanwharris
requested review from
ananyahjha93,
Borda,
carmocca,
edenlightning,
justusschock,
kaushikb11 and
tchaton
as code owners
ethanwharris
changed the title
flash.core.data.io.outputflash.core.data.io.output
Codecov Report
@@ Coverage Diff @@
## master #927 +/- ##
===========================================
- Coverage 88.33% 76.02% -12.31%
===========================================
Files 241 242 +1
Lines 13017 13053 +36
===========================================
- Hits 11498 9924 -1574
- Misses 1519 3129 +1610
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
ethanwharris
changed the title
flash.core.data.io.outputflash.core.data.io.output
| @@ -67,7 +67,7 @@ def __init__( | |||
| metrics=metrics, | |||
| learning_rate=learning_rate, | |||
| multi_label=multi_label, | |||
| serializer=serializer or Labels(), | |||
| output=output or Labels(), | |||
There was a problem hiding this comment.
Personal preference, but I would prefer LabelsOutput as we would have InputImagePaths. This would make it more consistent and readable too.
There was a problem hiding this comment.
I think we should leave that for a potential follow up as we would need to deprecate Labels etc. rather than just rename as they are a public API.
| @@ -90,7 +90,7 @@ def _visualize(self, labels): | |||
| plt.imshow(labels_vis) | |||
| plt.show() | |||
|
|
|||
| def serialize(self, sample: Dict[str, torch.Tensor]) -> torch.Tensor: | |||
| def transform(self, sample: Dict[str, torch.Tensor]) -> torch.Tensor: | |||
There was a problem hiding this comment.
@ethanwharris do you think this adds confusion, naming serialize to transform, since we have transform being used at other places.
There was a problem hiding this comment.
Maybe, although it definitely shouldn't be called serialize any more. As the role of Output is to convert raw predictions into the desired output format I think transform is fine. But it could otherwise be something more explicit like convert(raw_predictions) rather than transform(sample)
There was a problem hiding this comment.
I was thinking about export_transform.
…hLightning/lightning-flash into feature/rename_serializer
| # 3. Attach the Serializer | ||
| model.serializer = Probabilities() | ||
| # 3. Attach the Output | ||
| model.output = Probabilities() |
There was a problem hiding this comment.
Should we call them ProbabilitiesOutput ?
What does this PR do?
Fixes #917
Before submitting
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 🙃