Tests on more real datasets for some datasets #707
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zhiltsov-max
reviewed
Apr 4, 2022
Comment on lines
+18
to
+20
| MANGLING_DATASET_DIR = osp.join( | ||
| osp.dirname(__file__), "assets", "widerface_dataset", "mangling_dataset" | ||
| ) |
There was a problem hiding this comment.
- What is the difference between mangled datasets and example datasets? Do we need them all?
- I see that annotations in the mangled datasets are incorrect now, because the images are resized. It doesn't seem right. From the disk size view, single-color big images should take the same amount of memory as the small ones, so it shouldn't be a problem.
- Please rename the directory from
mangling_...tomangled_...
zhiltsov-max
reviewed
Apr 4, 2022
| @@ -398,8 +403,50 @@ def test_inplace_save_writes_only_updated_data(self): | |||
| ignored_attrs=IGNORE_ALL, | |||
| ) | |||
|
|
|||
| @mark_requirement(Requirements.DATUM_GENERAL_REQ) | |||
There was a problem hiding this comment.
Please add an extra category for these tests. Maybe, they should even be extracted into a separate directory.
zhiltsov-max
reviewed
Apr 4, 2022
Comment on lines
+435
to
+446
| def test_can_filter_by_subsets(self): | ||
| source_dataset = Dataset.import_from(MANGLING_DATASET_DIR, "wider_face") | ||
|
|
||
| first_dataset = source_dataset.filter("/item/annotation[label='face']") | ||
| second_dataset = source_dataset.filter("/item/annotation[label!='face']") | ||
|
|
||
| merger = IntersectMerge() | ||
| merged_dataset = merger([first_dataset, second_dataset]) | ||
|
|
||
| compare_datasets( | ||
| self, source_dataset, merged_dataset, require_media=True, ignored_attrs={"score"} | ||
| ) |
There was a problem hiding this comment.
This test doesn't seem to be aligned with its name. Why the merging is being done? Why the subsets are obtained by label filtering?
zhiltsov-max
reviewed
Apr 4, 2022
| @@ -398,8 +403,50 @@ def test_inplace_save_writes_only_updated_data(self): | |||
| ignored_attrs=IGNORE_ALL, | |||
| ) | |||
|
|
|||
| @mark_requirement(Requirements.DATUM_GENERAL_REQ) | |||
| def test_can_convert_to_widerface(self): | |||
There was a problem hiding this comment.
It's not really a "conversion", so maybe it should be renamed. Also, it's not really clear why this test is needed, maybe some comments or the test category description should be added.
zhiltsov-max
reviewed
Apr 4, 2022
| parsed_dataset, | ||
| require_media=True, | ||
| ignored_attrs={"difficult", "truncated", "occluded"}, | ||
| externally_comparison=True, |
There was a problem hiding this comment.
Suggested change
| externally_comparison=True, | |
| external_comparison=True, |
Can we remove this parameter completely?
zhiltsov-max
reviewed
Apr 4, 2022
| @@ -137,15 +140,49 @@ def _compare_annotations(expected, actual, ignored_attrs=None): | |||
| return r | |||
|
|
|||
|
|
|||
| def _compare_annotations_externally(expected, actual, ignored_attrs=None): | |||
There was a problem hiding this comment.
Can you reuse the regular comparison function?
zhiltsov-max
reviewed
Apr 4, 2022
| def compare_datasets( | ||
| test, | ||
| expected: IDataset, | ||
| actual: IDataset, | ||
| ignored_attrs: Union[None, Literal["*"], Collection[str]] = None, | ||
| require_media: bool = False, | ||
| require_images: bool = False, | ||
| externally_comparison=False, |
There was a problem hiding this comment.
It's not clear what external means here.
zhiltsov-max
reviewed
Apr 4, 2022
Comment on lines
+227
to
+238
| if externally_comparison: | ||
| ann_b = find( | ||
| ann_b_matches, | ||
| lambda x: _compare_annotations_externally( | ||
| x, ann_a, ignored_attrs=ignored_attrs | ||
| ), | ||
| ) | ||
| else: | ||
| ann_b = find( | ||
| ann_b_matches, | ||
| lambda x: _compare_annotations(x, ann_a, ignored_attrs=ignored_attrs), | ||
| ) |
There was a problem hiding this comment.
cmp_anns = ...
...
cmp_anns(a, b, ...)I suggest to use the strategy pattern here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
compare_datasetsfunction:idandgroupof annotationsQuestions:
How to test
Checklist
developbranchLicense
Feel free to contact the maintainers if that's a concern.