-
Notifications
You must be signed in to change notification settings - Fork 212
more return types #195
more return types #195
Conversation
Codecov Report
@@ Coverage Diff @@
## master #195 +/- ##
==========================================
+ Coverage 76.52% 76.58% +0.06%
==========================================
Files 56 56
Lines 2334 2345 +11
==========================================
+ Hits 1786 1796 +10
- Misses 548 549 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
return_type: str = None, | ||
labels: Optional[List[str]] = None, |
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.
is it updated also in docs?
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.
@Borda are docs created automatically with help of docstrings?
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.
yes, types and default are also in docs
@aniketmaurya mind check collision :] |
Sure @Borda |
Note that this should be handled inside a |
@carmocca are you suggesting to put |
DataPipeline was completely revamped recently. It now includes Preprocess and Postprocess objects. These return conversions are now expected to be encapsulated by the postprocess part. What is your use case exactly? For example: cc: @tchaton |
@@ -55,6 +56,11 @@ class Task(LightningModule): | |||
learning_rate: Learning rate to use for training, defaults to `5e-5` | |||
""" | |||
|
|||
class Return(Enum): |
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.
I like the idea, but the PR is outdate. Do you mind merging with master. Also, this logic should be handled by the postprocess object.
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.
Hi @aniketmaurya, great work on this PR 😃 I've now added multiple return types in #229 (with some inspiration from your work) so closing this. Please feel free to provide your thoughts on #229 |
What does this PR do?
Fixes #191
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 🙃