Update aggregator.py #2995
Open
Update aggregator.py #2995
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
l-bat
reviewed
Sep 30, 2024
| @@ -82,6 +84,11 @@ def collect_statistics(self, model: TModel, graph: NNCFGraph) -> None: | |||
| empty_statistics = False | |||
| if empty_statistics: | |||
| raise nncf.ValidationError(EMPTY_DATASET_ERROR) | |||
|
|
|||
| if len(self.dataset) < subset_size: | |||
There was a problem hiding this comment.
There is no guarantee that __len__ is implemented for the dataset. In the Statistics collection loop, you can count the number of processed input_data, and after the loop, you can check if subset_size is higher than the number of samples in the provided dataset. I think we should display a warning instead of an error in this case.
l-bat
reviewed
Oct 4, 2024
| @@ -23,6 +23,8 @@ | |||
| from nncf.data.dataset import DataItem | |||
| from nncf.data.dataset import Dataset | |||
| from nncf.data.dataset import ModelInput | |||
| import warnings | |||
There was a problem hiding this comment.
Suggested change
| import warnings | |
| from nncf.common.logging import nncf_logger |
| raise nncf.ValidationError(EMPTY_DATASET_ERROR) | ||
|
|
||
| if subset_size > processed_samples: | ||
| warnings.warn(f"Dataset contains only {processed_samples} samples, smaller than the requested subset size {subset_size}.") |
There was a problem hiding this comment.
Suggested change
| warnings.warn(f"Dataset contains only {processed_samples} samples, smaller than the requested subset size {subset_size}.") | |
| nncf_logger.warning(f"Dataset contains only {processed_samples} samples, smaller than the requested subset size {subset_size}.") |
Comment on lines
+37
to
+38
|
|
||
|
|
There was a problem hiding this comment.
Please avoid making unnecessary changes
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.
Changes
Added an error message
Reason for changes
send warning message to avoid Inconsistencies arise when the dataset size is less than the provided or default 'subset_size'.
Related tickets
Closes: #2562
I had an inquiry:
I noticed that subset_size is sometimes put as 100, or 300, or specified in the advanced parameters. Should a default be used here, or could you point me to where I can find the correct subset_size to be imported?