-
Notifications
You must be signed in to change notification settings - Fork 9
Yolov8 parse roboflow datasets #28
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
Conversation
|
Same fix is also used by ultralytics: https://github.com/ultralytics/ultralytics/blob/da9d8730f9f8a99fe6cd980fc927fd1005c15676/ultralytics/data/utils.py#L312 |
MalteEbner
left a comment
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.
While this fixes it for the case of roboflow datasets, it makes it fail for cases where the ../ is intended. Are you sure this is what we want?
Suggestion: Handle it the way ultralytics does it.
MalteEbner
left a comment
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.
LGTM
tests/unit/formats/test_yolov8.py
Outdated
| return _create_config | ||
|
|
||
|
|
||
| class TestYOLOv8BaseInput: |
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.
Please keep the leading underscore. This makes it easy to find the corresponding test class for every class via STRG-F. The test class should always be named Test{ClassName} while keeping all leading and trailing underscores.
See our style guide: https://www.notion.so/getlightly/Python-testing-78e2dc49baf340178fa44ce20031c52e
| class TestYOLOv8BaseInput: | |
| class Test_YOLOv8BaseInput: |
tests/unit/formats/test_yolov8.py
Outdated
|
|
||
|
|
||
| class TestYOLOv8BaseInput: | ||
| class TestGetCategories: |
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.
Having a class within a class is something I haven't seen yet. I think it is ok. However, I would have preferred the keeping the usual naming of
test_get_categories_from_dict_format
test_get_categories_from_list_format
test_get_categories_from_yaml_block_format
test_get_categories_invalid_namesThat would make the tests shorter and remove the extra class in a class. It would also allow to fin the tests corresponding to a method faster via STRG-F.
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.
Same for the other classes within classes
Feel free to resolve.
Changes
.yamlfile such aswith data.yaml content of:
How it has been tested