feat!: clarify eval strategy arguments #85
Merged
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
ThibaultFy
changed the title
ThibaultFy
force-pushed
the
clarify-eval-strat
branch
from
50179e2
to
32db613
Compare
oleobal
reviewed
Feb 7, 2023
substrafl/evaluation_strategy.py
Outdated
| if not isinstance(rounds, (list, int)): | ||
| raise TypeError(f"rounds must be of type list of ints or an int, {type(rounds)} found") | ||
| if eval_frequency is None and eval_rounds is None: | ||
| raise ValueError("At least one of eval_frequency or eval_rounds must be define") |
There was a problem hiding this comment.
Suggested change
| raise ValueError("At least one of eval_frequency or eval_rounds must be define") | |
| raise ValueError("At least one of eval_frequency or eval_rounds must be defined") |
oleobal
reviewed
Feb 7, 2023
substrafl/evaluation_strategy.py
Outdated
Comment on lines
184
to
185
| elif not all(isinstance(r, int) for r in self._eval_rounds): | ||
| raise TypeError(f"eval_rounds must be of type list of ints, {type(self._eval_rounds)} found") |
There was a problem hiding this comment.
Nitpick: isn't type(self._eval_rounds) always going to be list? Maybe we could have a more helpful error message:
Suggested change
| elif not all(isinstance(r, int) for r in self._eval_rounds): | |
| raise TypeError(f"eval_rounds must be of type list of ints, {type(self._eval_rounds)} found") | |
| elif not all(isinstance(r, int) for r in self._eval_rounds): | |
| raise TypeError(f"eval_rounds must be of type list of ints, {next(type(x) for x in self._eval_rounds if not isinstance(x, int))} found") |
(by the way, maybe we could test for collections.abc.Iterable rather than just list)
There was a problem hiding this comment.
Thanks for the catch, I agree :)
I print all the list in case there is more than one element with the wrong type :) .
oleobal
approved these changes
Feb 8, 2023
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
ThibaultFy
force-pushed
the
clarify-eval-strat
branch
from
a67046f
to
0c58b3b
Compare
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.
Signed-off-by: ThibaultFy 50656860+ThibaultFy@users.noreply.github.com
Related issue
https://app.asana.com/0/1203124674173179/1203493303699138/f
#followed by the number of the issueSummary
Notes
Please check if the PR fulfills these requirements