Add NDR #113
Add NDR #113
Conversation
CHANGELOG.md
Outdated
| @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - `Icdar13/15` dataset format (<https://github.com/openvinotoolkit/datumaro/pull/96>) | |||
| - Laziness, source caching, tracking of changes and partial updating for `Dataset` (<https://github.com/openvinotoolkit/datumaro/pull/102>) | |||
| - `Market-1501` dataset format (<https://github.com/openvinotoolkit/datumaro/pull/108>) | |||
| - Add NDR (<https://github.com/openvinotoolkit/datumaro/pull/113>) | |||
There was a problem hiding this comment.
Please use the full form here instead of abbreviation to make it more clear for readers.
There was a problem hiding this comment.
Okay, I will update.
| def __init__(self, extractor, working_subset=None, duplicated_subset="duplicated", algorithm='gradient', | ||
| num_cut=None, over_sample='random', under_sample='uniform', seed=None, **kwargs): | ||
| """ | ||
| Near-duplicated image removal |
There was a problem hiding this comment.
Hi, please include a couple of examples in the class doc and maybe some explanation for the CLI parameters, if this function is supposed to be used from CLI. An example can be found here: https://github.com/openvinotoolkit/datumaro/blob/develop/datumaro/plugins/transforms.py#L313
There was a problem hiding this comment.
I planned to support CLI by inheriting CliPlugin in another PR. Do I need to write a documentation anyway?
There was a problem hiding this comment.
Let's add it in another PR then.
|
|
||
| Arguments | ||
| --------------- | ||
| working_subset: str |
There was a problem hiding this comment.
I'm not sure if using a parameter is better than passing just a subset as an input. Also, I think, it looks more natural, when None run the algorithm for the whole dataset.
There was a problem hiding this comment.
I think that might return unexpected results if user has a subset like val or test.
datumaro/plugins/ndr.py
Outdated
| raise ValueError("Invalid under_sample") | ||
|
|
||
| self.duplicated_subset = duplicated_subset | ||
| self._remove(algorithm, working_subset, num_cut, over_sample, under_sample, **kwargs) |
There was a problem hiding this comment.
Try to make the transform lazy - it should only be applied when __iter__ is called, look at "splitter" transforms for example. You'll also need to move the initialization of RNG into __iter__. Consider __iter__ in the input dataset a heavy operation.
There was a problem hiding this comment.
Okay, I will check "splitter" and change to lazy transformation
datumaro/plugins/ndr.py
Outdated
| super().__init__(extractor) | ||
| if seed: | ||
| np.random.seed(seed) | ||
| if len(extractor) < 1: |
There was a problem hiding this comment.
I don't think this situation should be considered an error.
There was a problem hiding this comment.
Okay, I will update
datumaro/plugins/ndr.py
Outdated
| if num_cut and num_cut > len(all_imgs): | ||
| raise ValueError("The number of images is smaller than the cut you want") | ||
|
|
||
| if algorithm == "gradient": |
There was a problem hiding this comment.
Consider using an enum instead of a string constant.
There was a problem hiding this comment.
Could you explain more about enum?
class Algo_name(Enum):
algo_name1 = "..."
....
...
if algorithm == Algo_name.algo_name1Do you mean creating a class by inheriting Enum like above?
There was a problem hiding this comment.
AlgoName = Enum('AlgoName', ['good', 'best', 'poor'])Examples: https://github.com/openvinotoolkit/datumaro/blob/develop/datumaro/plugins/voc_format/format.py#L16, https://github.com/openvinotoolkit/datumaro/blob/develop/datumaro/cli/contexts/project/__init__.py#L228
datumaro/plugins/ndr.py
Outdated
| continue | ||
|
|
||
| # Hash collision: compare dot-product based feature similarity | ||
| max_sim = np.max(np.dot(clr_dict[key], clr) ** 50) |
There was a problem hiding this comment.
Please make the use of 50 more clear. It is unclear if it is just a random number, or something with a strong mathematical proof.
There was a problem hiding this comment.
We do not have any experimental results for that number. The number is for maximizing the gap between duplicated-images and non-duplicated-images and is set arbitrarily. Then do we need to change it as a parameter?
There was a problem hiding this comment.
I don't have any complaints against the value, but I'd better make it a named constant to clarify its use.
There was a problem hiding this comment.
Oh I understand. Okay, I will update
Summary
Added NDR to remove duplicated images in a dataset.
How to test
python -m unittest -v tests/test_ndr.pyChecklist
developbranchLicense
Feel free to contact the maintainers if that's a concern.