-
Notifications
You must be signed in to change notification settings - Fork 135
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
Refactor operations #901
Refactor operations #901
Conversation
- Move annotation matchers and mergers to the seperate directory - Move get_merger to the seperate directory
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #901 +/- ##
===========================================
+ Coverage 78.88% 78.89% +0.01%
===========================================
Files 204 208 +4
Lines 25020 25037 +17
Branches 5031 5031
===========================================
+ Hits 19736 19753 +17
Misses 4152 4152
Partials 1132 1132
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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.
Looks good to me.
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.
Why did you introduce annotations directory and what stands for? We may have prior understandings to make the same consensus. Could you explain?
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.
Are you thinking about introducing merge directory as a python API refactoring manner?
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
First of all, this file has too many lines (2007 lines) in it: https://github.com/openvinotoolkit/datumaro/blob/develop/datumaro/components/operations.py. Therefore, many classes and functions for different purposes are defined in this file. This is the full list of definitions in this file: My motivation is to refactor merge related things to somewhere but not in |
I agree with separating Matcher and Merger from operations. But my question is about annotation directory. I would like to know what annotation directory means for Matcher and Merger. |
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
Since |
alright, I agree that matcher should be closely related to the annotation-level operation. But I think that merger is more about dataset-level operation. What do you think? Do you have any plan to create dataset directory in a similar vein? |
My plan was like this, and in this PR, 1) is completed and 2) is partially completed. |
Summary
How to test
Checklist
License
Feel free to contact the maintainers if that's a concern.