Remove CmaEsAttrKeys and _attr_keys for Simplification#6068
Remove CmaEsAttrKeys and _attr_keys for Simplification#6068HideakiImamura merged 2 commits intooptuna:masterfrom
CmaEsAttrKeys and _attr_keys for Simplification#6068Conversation
|
@HideakiImamura @c-bata Could you review this PR? |
optuna/samplers/_cmaes.py
Outdated
| params = optimizer.ask() | ||
|
|
||
| generation_attr_key = self._attr_keys.generation | ||
| generation_attr_key = self._attr_prefix + "generation" |
There was a problem hiding this comment.
Nit: It might be better to introduce the following two property methods to avoid inconsistencies in the key definitions.
@property
def _attr_key_generation(self) -> str:
return self._attr_prefix + "generation"
@property
def _attr_key_optimizer(self, index: int) -> str:
return f"{self._attr_prefix}optimizer:{index}"| key: value | ||
| for key, value in trial.system_attrs.items() | ||
| if key.startswith(self._attr_keys.optimizer) | ||
| if key.startswith(self._attr_key_optimizer) |
There was a problem hiding this comment.
Due to this implementation, the index is not passed as an argument to _attr_key_optimizer.
There was a problem hiding this comment.
I think we don't need to use self._attr_key_optimizer here. Instead, you can simply hardcode "cma:optimizer" in this line. Let me explain the background.
In older versions of Optuna, the CMA-ES optimizer was stored as a single system_attr without being split. However, this approach caused issues when using MySQL as the storage backend, so it was revised in the following PR to store the optimizer across multiple system_attrs:
#1833
To keep the backward compatibility, PR #1833 introduced these lines. But since more than four years have passed, it's likely that very few users are still affected by this logic, and we could even consider removing it (though it would be better to do so in a separate PR).
|
Thank you for the review. I made the update accordingly! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6068 +/- ##
==========================================
- Coverage 88.43% 88.40% -0.03%
==========================================
Files 206 207 +1
Lines 13910 13973 +63
==========================================
+ Hits 12301 12353 +52
- Misses 1609 1620 +11 ☔ View full report in Codecov by Sentry. |
HideakiImamura
left a comment
There was a problem hiding this comment.
Thanks for the follow-up! LGTM!
Motivation
CmaEsAttrKeyswere removed in #6025.Description of the Changes
CmaEsAttrKeysclass._attr_keysmethod and replaced its usage with inline logic where necessary.