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

IceVision integration #608

Merged
merged 64 commits into from
Aug 16, 2021
Merged

IceVision integration #608

merged 64 commits into from
Aug 16, 2021

Conversation

ethanwharris
Copy link
Collaborator

@ethanwharris ethanwharris commented Jul 20, 2021

What does this PR do?

Fixes #487
Fixes #480
Fixes #119

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 🙃

@pep8speaks
Copy link

pep8speaks commented Jul 20, 2021

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-08-12 12:41:35 UTC

@ethanwharris ethanwharris mentioned this pull request Jul 20, 2021
8 tasks
@ethanwharris ethanwharris added the enhancement New feature or request label Jul 20, 2021
flash/core/data/data_pipeline.py Outdated Show resolved Hide resolved
Comment on lines 46 to 79
class IceVisionParserDataSource(IceVisionPathsDataSource):

def __init__(self, parser: Optional[Type['Parser']] = None):
super().__init__()
self.parser = parser

def load_data(self, data: Tuple[str, str], dataset: Optional[Any] = None) -> Sequence[Dict[str, Any]]:
root, ann_file = data

if self.parser is not None:
parser = self.parser(ann_file, root)
dataset.num_classes = len(parser.class_map)
records = parser.parse(data_splitter=SingleSplitSplitter())
return [{DefaultDataKeys.INPUT: record} for record in records[0]]
else:
raise ValueError("The parser type must be provided")


class IceDataParserDataSource(IceVisionPathsDataSource):

def __init__(self, parser: Optional[Callable] = None):
super().__init__()
self.parser = parser

def load_data(self, data: Tuple[str, str], dataset: Optional[Any] = None) -> Sequence[Dict[str, Any]]:
root = data

if self.parser is not None:
parser = self.parser(root)
dataset.num_classes = len(parser.class_map)
records = parser.parse(data_splitter=SingleSplitSplitter())
return [{DefaultDataKeys.INPUT: record} for record in records[0]]
else:
raise ValueError("The parser must be provided")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could they be merged together ?

flash/core/integrations/icevision/model.py Outdated Show resolved Hide resolved
flash/core/integrations/icevision/model.py Outdated Show resolved Hide resolved
flash/image/detection/model.py Show resolved Hide resolved
flash/image/instance_segmentation/data.py Show resolved Hide resolved
flash/image/instance_segmentation/model.py Outdated Show resolved Hide resolved
flash_examples/graph_classification.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #608 (75e1c31) into master (d094fee) will decrease coverage by 0.57%.
The diff coverage is 84.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #608      +/-   ##
==========================================
- Coverage   90.58%   90.01%   -0.58%     
==========================================
  Files         173      185      +12     
  Lines        9232     9663     +431     
==========================================
+ Hits         8363     8698     +335     
- Misses        869      965      +96     
Flag Coverage Δ
unittests 90.01% <84.02%> (-0.58%) ⬇️

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

Impacted Files Coverage Δ
flash/core/serve/core.py 87.87% <ø> (ø)
flash/core/utilities/url_error.py 93.33% <50.00%> (-6.67%) ⬇️
flash/core/utilities/imports.py 89.09% <66.66%> (-0.63%) ⬇️
flash/core/integrations/icevision/transforms.py 69.44% <69.44%> (ø)
flash/image/instance_segmentation/data.py 71.87% <71.87%> (ø)
flash/image/keypoint_detection/data.py 72.41% <72.41%> (ø)
flash/image/detection/backbones.py 72.50% <72.50%> (ø)
flash/image/instance_segmentation/model.py 75.00% <75.00%> (ø)
flash/image/keypoint_detection/model.py 75.00% <75.00%> (ø)
flash/core/model.py 86.28% <81.94%> (-0.14%) ⬇️
... and 35 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 d094fee...75e1c31. Read the comment docs.

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

would it be possible to have the refactoring in separate PR (for example moving backbones) so it is easier to see what are the real changes?

.github/workflows/ci-testing.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,31 @@

.. _instance_segmentation:
Copy link
Member

Choose a reason for hiding this comment

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

we can have these labels automatically generated by docs

flash/core/adapter.py Outdated Show resolved Hide resolved
flash/core/data/data_module.py Show resolved Hide resolved
flash/core/integrations/icevision/adapter.py Outdated Show resolved Hide resolved
flash/core/integrations/icevision/adapter.py Show resolved Hide resolved
flash/core/model.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Awesome work !

flash/image/detection/model.py Show resolved Hide resolved
pass


class AdapterTask(Task):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the AdapterTask use the mixins ?

tests/image/detection/test_model.py Show resolved Hide resolved
@ananyahjha93
Copy link
Contributor

LGTM!

@mergify mergify bot removed the has conflicts label Aug 16, 2021
@ethanwharris ethanwharris enabled auto-merge (squash) August 16, 2021 16:59
@ethanwharris ethanwharris disabled auto-merge August 16, 2021 17:16
@ethanwharris ethanwharris merged commit d9dc2f0 into master Aug 16, 2021
@ethanwharris ethanwharris deleted the feature/icevision branch August 16, 2021 17:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
5 participants