-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: Add argument img_folder to OpenFire #34
Conversation
Following the discussion in pyronear#26, add argument (str or Path) to OpenFire. If not given, the default is used. The rest of the folder structure is kept for the moment. test_datasets.py now tests its type and also if the download succeeded approximately: up to 10 samples can be missing.
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
==========================================
+ Coverage 86.25% 86.53% +0.27%
==========================================
Files 21 21
Lines 866 869 +3
==========================================
+ Hits 747 752 +5
+ Misses 119 117 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Great PR!
Just a little edit for good pep8 compliance if you don't mind
Following comments on pyronear#32: - Avoiding long lines, also splitting if / else in OpenFire c-tor - Changing style and adding test on default img_folder value in test_datasets.py
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.
Very useful PR to symlink image folder located elsewhere for training!
A few extra tests will have to be added to unittests to monitor further developments of OpenFire but this one is working as is.
Adding argument --img-folder to fastai/train.py and torch/train.py
@blenzi Just checking, is this still on? Or was it covered by another PR recently? |
This branch remained idle for a long time. Trying to merge with frgfm/master. Short summary of new features: - Adding argument img_folder to OpenFire c-tor, stored in img_folder attribute (default to previous value: root/OpenFire/images) and returned by _images property - Adding argument --img-folder to fastai/train.py and torch/train.py - Associated tests
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.
Thanks for clearing the PR! Looks good to me
@blenzi : Do you need any help to resolve the conflicts ? |
Mind trying to solve the conflicts @blenzi 🙏 ? (the scripts were moved, just need some fixing) |
In test_datasets.py/DatasetsTester.test_openfire: allow up to 15 failed downloads instead of 10
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.
Thanks for fixing the PR!
Following the discussion in #26, add argument img_folder (str or Path) to OpenFire. If not given, the default
<root>/OpenFire/images
is used. The rest of the folder structure is kept for the moment.test_datasets.py now tests its type and also if the download succeeded approximately: up to 10 samples can be missing.