Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lib restructure #1120

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Lib restructure #1120

wants to merge 11 commits into from

Conversation

fstroth
Copy link
Contributor

@fstroth fstroth commented Jul 13, 2022

His PR refactors ross and ultralytics to the new structure.

@codecov
Copy link

codecov bot commented Jul 13, 2022

Codecov Report

Merging #1120 (94dcc47) into master (ba6a714) will increase coverage by 0.06%.
The diff coverage is 94.30%.

@@            Coverage Diff             @@
##           master    #1120      +/-   ##
==========================================
+ Coverage   85.48%   85.55%   +0.06%     
==========================================
  Files         305      293      -12     
  Lines        6675     6706      +31     
==========================================
+ Hits         5706     5737      +31     
  Misses        969      969              
Flag Coverage Δ
unittests 85.55% <94.30%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
...on/models/ross/efficientdet/backbones/backbones.py 100.00% <ø> (ø)
icevision/models/ross/efficientdet/lightning.py 36.66% <ø> (ø)
...n/models/ultralytics/yolov5/backbones/backbones.py 100.00% <ø> (ø)
icevision/models/ultralytics/yolov5/lightning.py 100.00% <ø> (ø)
icevision/models/ross/efficientdet/fastai.py 32.14% <35.71%> (ø)
.../models/fastai/unet/backbones/mobilenet_configs.py 100.00% <100.00%> (ø)
...ion/models/fastai/unet/backbones/resnet_configs.py 87.50% <100.00%> (+0.83%) ⬆️
...ion/models/ross/efficientdet/backbones/__init__.py 100.00% <100.00%> (ø)
icevision/models/ross/efficientdet/show_batch.py 50.00% <100.00%> (+5.55%) ⬆️
...vision/faster_rcnn/backbones/resnet_fpn_configs.py 88.00% <100.00%> (+5.14%) ⬆️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

frederik added 4 commits July 15, 2022 13:32
…f the backend lib, the model type, backbone name and the number of classes (optionaly configuration for the backbone and the model can be supplied). Some of the backbon files have been modified to work with the build_model function, the util files (that contained one or two functions where merged into the main backbone files to reduce clutter and remove edgecases in the handeling of model/backbone creation.


def build_model(
backend_lib_name: str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wdyt about simplifying the argument name by using:
lib_name, model_name instead of backend_lib_name and model_type_name

Also, would it be better to pass a dictionary instead of argument. Maybe using a dict, we will loose auto-completion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I will change the naming. I think keeping arguments is better as we have autocompletion with that approach and users still can use dicts if they want to and just unpack them with **dict into the function call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants