Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add BaseViz Callback (2 / 2) #201

Merged
merged 37 commits into from
Apr 6, 2021
Merged

Add BaseViz Callback (2 / 2) #201

merged 37 commits into from
Apr 6, 2021

Conversation

tchaton
Copy link
Contributor

@tchaton tchaton commented Mar 31, 2021

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

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests? [not needed for typos/docs]
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)

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 🙃

@tchaton tchaton self-assigned this Mar 31, 2021
@tchaton tchaton added this to the 0.2 milestone Mar 31, 2021
@tchaton tchaton changed the base branch from master to data_pipeline_current_fn March 31, 2021 17:35
@tchaton tchaton changed the title [feat] Add BaseViz Callback (2 / 2) [WIP] Add BaseViz Callback (2 / 2) [Don't merge] Mar 31, 2021
@tchaton tchaton changed the base branch from data_pipeline_current_fn to master March 31, 2021 17:36
@codecov
Copy link

codecov bot commented Mar 31, 2021

Codecov Report

Merging #201 (67ba94c) into master (6e548a4) will increase coverage by 0.82%.
The diff coverage is 97.48%.

Impacted file tree graph

@@            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     
Flag Coverage Δ
unittests 86.31% <97.48%> (+0.82%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flash/core/model.py 93.71% <ø> (-0.31%) ⬇️
flash/vision/classification/data.py 87.92% <ø> (+0.48%) ⬆️
flash/data/process.py 84.86% <91.66%> (+0.36%) ⬆️
flash/data/base_viz.py 95.58% <95.58%> (ø)
flash/data/data_module.py 78.49% <97.77%> (+5.95%) ⬆️
flash/core/utils.py 100.00% <100.00%> (ø)
flash/data/auto_dataset.py 96.25% <100.00%> (+0.47%) ⬆️
flash/data/batch.py 79.67% <100.00%> (+2.82%) ⬆️
flash/data/callback.py 100.00% <100.00%> (ø)
flash/data/data_pipeline.py 87.36% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e548a4...67ba94c. Read the comment docs.

@tchaton tchaton changed the base branch from master to data_pipeline_current_fn March 31, 2021 18:04
@tchaton tchaton changed the base branch from data_pipeline_current_fn to master March 31, 2021 18:05
@tchaton tchaton changed the base branch from master to data_pipeline_current_fn April 1, 2021 10:49
Base automatically changed from data_pipeline_current_fn to master April 1, 2021 11:33
@tchaton tchaton changed the base branch from master to base_viz April 1, 2021 11:33
@tchaton tchaton changed the base branch from base_viz to master April 1, 2021 11:33
flash/core/model.py Outdated Show resolved Hide resolved
flash/core/utils.py Outdated Show resolved Hide resolved
flash/core/model.py Outdated Show resolved Hide resolved
tests/data/test_data_viz.py Show resolved Hide resolved
test.py Outdated Show resolved Hide resolved
flash/vision/classification/data.py Outdated Show resolved Hide resolved
flash/vision/classification/data.py Outdated Show resolved Hide resolved
flash/vision/classification/data.py Outdated Show resolved Hide resolved
"""Called after `per_batch_transform_on_device` """


class ControlFlow(FlashCallback):
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@edgarriba edgarriba left a 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

flash/core/utils.py Outdated Show resolved Hide resolved
flash/data/auto_dataset.py Show resolved Hide resolved
flash/data/auto_dataset.py Show resolved Hide resolved
flash/data/auto_dataset.py Outdated Show resolved Hide resolved
flash/data/callback.py Outdated Show resolved Hide resolved
flash/data/callback.py Outdated Show resolved Hide resolved
_ = 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:
Copy link
Contributor

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.

Copy link
Contributor Author

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.

flash/data/process.py Outdated Show resolved Hide resolved
flash/data/process.py Outdated Show resolved Hide resolved
tests/data/test_auto_dataset.py Outdated Show resolved Hide resolved
@Borda Borda changed the title [WIP] Add BaseViz Callback (2 / 2) [Don't merge] [WIP] Add BaseViz Callback (2 / 2) Apr 6, 2021
@Borda Borda marked this pull request as draft April 6, 2021 13:51
@tchaton tchaton marked this pull request as ready for review April 6, 2021 15:01
Copy link
Contributor

@edgarriba edgarriba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

flash/data/base_viz.py Show resolved Hide resolved
@tchaton tchaton changed the title [WIP] Add BaseViz Callback (2 / 2) Add BaseViz Callback (2 / 2) Apr 6, 2021
@tchaton tchaton merged commit 6a4948a into master Apr 6, 2021
@tchaton tchaton deleted the base_viz_2 branch April 6, 2021 20:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants