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

Argparse #45

Merged
merged 8 commits into from
Dec 10, 2018
Merged

Argparse #45

merged 8 commits into from
Dec 10, 2018

Conversation

guigarfr
Copy link
Contributor

@guigarfr guigarfr commented Dec 5, 2018

I'm experimenting with your code with a single class dataset.

I made some changes which make the code clearer, i will send you PRs in case you find it interesting.

Here the main changes are:

  • created some methods in torch_utils.py (used in the three scripts) so code is not repeated
  • in your version, the arguments stored in opt were imported and "shared" between the scripts (train, which imported test). That is not a good practice with argparse. I moved the argparse code to the main block and called the test method from inside train using the required arguments.
  • as in the argparse doc, i used double -- for long names (- are used for short versions like -h being --help)
  • the boolean options i changed them to action store_true, so there's no need to do --report 1, just --report will activate the reporting and the absence of the argument would mean reporting is not activated.
  • changed the doc accordingly
  • used the names already defined in the data config file, instead of adding an extra parameter. makes no sense to use the data from the file, but not the label names...

You can check the changes commit by commit. I tried to make the commits compact, with a correct message and understandable

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Updated training and testing commands, improved detect function, and code refactoring in YOLOv3.

📊 Key Changes

  • Unified command line argument syntax, replacing single dash - with double dashes --.
  • Replaced detect.py script with a detect() function.
  • Improved test.py and train.py, organizing them to be more modular and have simplified arguments.
  • Added torch_utils.py for handling device selection and seed initialization.
  • Overall cleanup and removal of redundant code.

🎯 Purpose & Impact

  • Simplifying Command Usage: Users can expect a consistent command-line interface with standard double dash argument syntax, preventing confusion and errors.
  • Detect Function Refactor: By shifting from a script with global variables to a function, the detect() method modularizes the code, making it cleaner, easier to understand, and reusable.
  • Improved Modularity in Testing/Training: test.py and train.py have been refactored to functions accepting arguments, improving readability and making automated testing/training easier to implement.
  • Centralized Device and Seed Management: The creation of torch_utils.py centralizes common functions, thereby reducing code duplication and potential inconsistencies across scripts.
  • Overall Impact: These changes aim to enhance the maintainability of the code, reduce errors, and provide a more user-friendly experience for those working with the YOLOv3 implementation by Ultralytics.

@guigarfr guigarfr force-pushed the argparse branch 2 times, most recently from 9efc5da to 3235260 Compare December 5, 2018 15:54
Keep parsing inside __main__ block and call methods with arguments

Add double -- for long argument names (- reserved for shortcuts)
Default is false.

If want to resume, call train.py --resume
Default is false: python train.py
If want the report: python train.py --report
Shorten name of --data-config-path argument to --data-config
If I want to store my weights in 'weights2' path:
python train.py --weights-path weights2

Default is the same: weights
Traing with freeze: python train.py --freeze
Train without freeze: python train.py

Note: in the actual version freeze is only for first epoche
@glenn-jocher
Copy link
Member

@guigarfr thanks for the PR! I actually only started using python this year so thank you for the coding recommendations. I'll try and review it soon and get it implemented.

@guigarfr
Copy link
Contributor Author

guigarfr commented Dec 9, 2018

Sure, feel free to review it and comment the PR.

As I said before, the commit messages are very descriptive.

@glenn-jocher glenn-jocher merged commit 362b414 into ultralytics:master Dec 10, 2018
@guigarfr guigarfr deleted the argparse branch December 11, 2018 09:08
chrizandr pushed a commit to chrizandr/yolov3 that referenced this pull request Aug 19, 2019
@glenn-jocher
Copy link
Member

@guigarfr sounds great! I will make sure to review the commit messages and provide feedback on the PR. Thanks again for your contribution!

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.

None yet

2 participants