change checks to init - update - compute structure #826
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
noamzbr
requested review from
matanper,
ItayGabbay,
JKL98ISR,
shir22,
benisraeldan,
yromanyshyn and
nirhutnik
matanper
reviewed
Feb 7, 2022
|
so all checks are built for batches in their design like the ignite metrics? |
Basically yes, I just named our "reset" to initialize_run |
matanper
reviewed
Feb 8, 2022
matanper
reviewed
Feb 8, 2022
JKL98ISR
requested changes
Feb 8, 2022
Crashes due to vision/base.py line 199
This reverts commit a392878.
Also add to formatter docs that class_id must be location in prediction vector (in classification)
JKL98ISR
approved these changes
Feb 9, 2022
matanper
reviewed
Feb 9, 2022
| @@ -152,12 +135,60 @@ def assert_task_type(self, *expected_types: TaskType): | |||
| f'Check is irrelevant for task of type {self.train.task_type}') | |||
| return True | |||
|
|
|||
| def infer(self, batch: Any) -> Any: | |||
There was a problem hiding this comment.
to cache the predictions for a given batch. Used for e.g. in performance report.
Refactor to proper init-update-compute structure will follow shortly
matanper
reviewed
Feb 9, 2022
| checks_list = [] | ||
| for check_idx, check in list(self.checks.items()): | ||
| if isinstance(check, (TrainTestCheck, ModelOnlyCheck)): | ||
| checks_list.append((check_idx, copy.deepcopy(check))) |
There was a problem hiding this comment.
why do you need to copy here?
There was a problem hiding this comment.
Because checks have state, seems cleaner to me to copy them when doing a specific .run() call rather than use the originals used in the constructor
matanper
reviewed
Feb 9, 2022
| results = OrderedDict({check_idx: None for check_idx, check in check_dict.items()}) | ||
|
|
||
| # Loop over training batches | ||
| n_batches = len(context.train.get_data_loader()) |
There was a problem hiding this comment.
so we decided data loader must be mapped?
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.
This PR is intended to change the structure of our vision checks to use the following methods:
initialize_run - everything that should be done before we start running on the batches returned by dataloader
update - accumulate results for a specific batch
compute - return the final CheckResult using accumulated values.
Updated also the base checks in vision to match, where now run_logic handels running all these functions for a call of a single check.
This is a draft, please let me know what you think. If it's approved I'll expand these changes to suite and to the label drift check.