Add cached extra study property #217
Conversation
| if self.has_intermediate_values: | ||
| return |
There was a problem hiding this comment.
Even if self.has_intermediate_values is True, we need to continue for-loop to update the search space cache.
| @@ -65,11 +72,15 @@ def update(self, trials: List[FrozenTrial]) -> None: | |||
| if trial.state not in states_of_interest: | |||
| continue | |||
|
|
|||
| if len(trial.intermediate_values) > 0: | |||
There was a problem hiding this comment.
[nits] How about changing here like following to reduce a bit of computation since we don't need to update self.has_intermediate_values if it's already True?
Deleted
There was a problem hiding this comment.
Thank you very much.
When self.has_intermediate_values is True , there is no need to run L76, so
Isn't this place if not self.has_intermediate_values and len(trial.intermediate_values) > 0:
There was a problem hiding this comment.
Oh, sorry. You are completely correct 🙏
| def test_no_intermediate_value(self) -> None: | ||
| intermediate_values = [ | ||
| {}, | ||
| {}, | ||
| ] | ||
| trials = [ | ||
| create_trial( | ||
| state=TrialState.COMPLETE, | ||
| value=0, | ||
| distributions={"x0": UniformDistribution(low=0, high=10)}, | ||
| intermediate_values=iv, | ||
| params={"x0": 0.5}, | ||
| ) | ||
| for iv in intermediate_values | ||
| ] | ||
| cached_extra_study_property = _CachedExtraStudyProperty() | ||
| cached_extra_study_property.update(trials) | ||
| self.assertFalse(cached_extra_study_property.has_intermediate_values) | ||
|
|
||
| def test_some_trials_has_no_intermediate_value(self) -> None: | ||
| intermediate_values = [ | ||
| {0: 0.3, 1: 1.2}, | ||
| {}, | ||
| {0: 0.3, 1: 1.2}, | ||
| ] | ||
| trials = [ | ||
| create_trial( | ||
| state=TrialState.COMPLETE, | ||
| value=0, | ||
| distributions={"x0": UniformDistribution(low=0, high=10)}, | ||
| intermediate_values=iv, | ||
| params={"x0": 0.5}, | ||
| ) | ||
| for iv in intermediate_values | ||
| ] | ||
| cached_extra_study_property = _CachedExtraStudyProperty() | ||
| cached_extra_study_property.update(trials) | ||
| self.assertTrue(cached_extra_study_property.has_intermediate_values) | ||
|
|
||
| def test_all_trials_has_intermediate_value(self) -> None: | ||
| intermediate_values = [{0: 0.3, 1: 1.2}, {0: 0.3, 1: 1.2}] | ||
| trials = [ | ||
| create_trial( | ||
| state=TrialState.COMPLETE, | ||
| value=0, | ||
| distributions={"x0": UniformDistribution(low=0, high=10)}, | ||
| intermediate_values=iv, | ||
| params={"x0": 0.5}, | ||
| ) | ||
| for iv in intermediate_values | ||
| ] | ||
| cached_extra_study_property = _CachedExtraStudyProperty() | ||
| cached_extra_study_property.update(trials) | ||
| self.assertTrue(cached_extra_study_property.has_intermediate_values) | ||
|
|
||
| def test_no_trials(self) -> None: | ||
| trials = [] | ||
| cached_extra_study_property = _CachedExtraStudyProperty() | ||
| cached_extra_study_property.update(trials) | ||
| self.assertFalse(cached_extra_study_property.has_intermediate_values) |
There was a problem hiding this comment.
How about splitting these test cases into another TestCase class? This test class looks somewhat large. These logics are defined in the same class (_CachedExtraStudyProperty) though.
There was a problem hiding this comment.
Hi @yoshinobc.
Thank you for your pull request!
You changes looks almost good after solving lint errors and my suggestions.
|
Merged! Thank you for your contribution 👍 |
Contributor License Agreement
This repository (
optuna-dashboard) and Goptuna share common code.This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Motivation
Eliminate unnecessary calculations when retrieving cached study properties.
Description of the changes
get_chached_extra_study_propertymethod in_app.py_search_space.pyand_intermediate_values.py. Update both information in a single update.