-
Notifications
You must be signed in to change notification settings - Fork 212
Conversation
Codecov Report
@@ Coverage Diff @@
## master #201 +/- ##
==========================================
+ Coverage 85.49% 86.31% +0.82%
==========================================
Files 54 56 +2
Lines 2495 2674 +179
==========================================
+ Hits 2133 2308 +175
- Misses 362 366 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
"""Called after `per_batch_transform_on_device` """ | ||
|
||
|
||
class ControlFlow(FlashCallback): |
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.
This is a bit weird to me as we don't have such thing in Lightning. We just loop over all callbacks for each hook around the code. Do you prefer this object? Why?
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, I prefer this object and I think we should add a similar one within Lightning. The callbacks logic should be fully managed by callbacks.
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.
Minor typos and typing
_ = next(iter(self.predict_dataloader())) | ||
|
||
def visualize(self, batch: Dict[str, Any], stage: RunningStage) -> None: | ||
def show(self, batch: Dict[str, Any], stage: RunningStage) -> 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.
just to leave here as a comment what I suggested in the call: This method and all the other methods callings that one just collect or fetch a batch. I would suggest later to rename to fetch
. Visualization is just one use case once you have cached the data. A general purpose thing could be profiling or data inspection.
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.
Let's change in another PR with team approval.
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.
LGTM
What does this PR do?
Next to #200
This PR adds a
BaseViz Callback
which wrap the preprocess hooks and gather batches of data.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 🙃