-
Notifications
You must be signed in to change notification settings - Fork 212
Refactor preprocess_cls
to preprocess
, add Serializer
, add DataPipelineState
#229
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov Report
@@ Coverage Diff @@
## master #229 +/- ##
==========================================
+ Coverage 86.57% 86.81% +0.24%
==========================================
Files 58 58
Lines 2912 2981 +69
==========================================
+ Hits 2521 2588 +67
- Misses 391 393 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@ethanwharris Could you please add some description about the PR. |
@kaushikb11 will do, only a draft at the moment 😃 |
Hello @ethanwharris! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-04-22 10:02:35 UTC |
Co-authored-by: thomas chaton <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat tests :)
Co-authored-by: Edgar Riba <[email protected]>
…TorchLightning/lightning-flash into feature/multiple_return_types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing PR :)
What does this PR do?
Bit of a big one. Makes the following changes:
{pre/post}process_cls
to just handing around references to instantiated objects. This includes:instantiate_preprocess
(now just handled in the__init__
of the class)ImageClassification
preprocess rather than the datamoduleSerializer
class so that a datamodule is now made up of aPreprocess
,Postprocess
, and aSerializer
. These are classes which control the conversion from model output to desired prediction format (e.g. labels, classes etc.). They are seperate from thePostprocess
as the user should be able to change theSerializer
without messing with the more fundamental stuff in thePostprocess
(without which the traning would usually fail). This Fixes Support more return types in predict #191 .DataPipelineState
. Children of the data pipeline need to be able to communicate, this expands thePreprocessState
to be shared across preprocess, postprocess, and serializer. This enables e.g.Labels
serializer just gets the labels from theClassificationState
registered in theload_data
of theImageClassificationPreprocess
.data_fetcher
as an argument tofrom_load_data_inputs
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 🙃