Fix CmaEs system attribution key

[gen740]

Motivation

Related to #4016 and #4142.

As noted in #4142, CmaEs's trial attribution is not properly working. Keys are different in Setting and Getting. This PR will fix this issue.

Description of the changes

• Add a new private property method _attr_keys to CmaEsSampler
• Using NamedTuple to confirm the _attr_keys's key
• Let CmaEsSampler's _concat_optimizer_attrs and _split_optimizer_str member function, since it require use_separable_cma and with_margin information.
• Rewrite cmaes_test. (This is WIP)

Could anyone explain what backward compatibility means?

[c-bata]

@eukaryo Could you review this PR after passed CI?

[codecov-commenter]

Codecov Report

Merging #4184 (8bef4de) into master (db1837c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4184   +/-   ##
=======================================
  Coverage   89.76%   89.77%           
=======================================
  Files         162      162           
  Lines       12621    12632   +11     
=======================================
+ Hits        11329    11340   +11     
  Misses       1292     1292           

Impacted Files	Coverage Δ
optuna/samplers/_cmaes.py	98.67% <100.00%> (+0.06%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[github-actions]

This pull request has not seen any recent activity.

[eukaryo]

Sorry for the late reply. I am OK to be assigned as a reviewer. However, I think it would be better to have another reviewer with more experience.

[c-bata]

Thank you for your pull request. I left two questions.

[c-bata]

The following code was prepared for users who used CmaEsSampler in prior to Optuna v2.5, but it seems old enough, so what do you think about removing it in this PR?

optuna/optuna/samplers/_cmaes.py

Lines 490 to 496 in 8bef4de

	if (
	not self._use_separable_cma
	and not self._with_margin
	and "cma:optimizer" in optimizer_attrs
	):
	# Check "cma:optimizer" key for backward compatibility.
	optimizer_str = optimizer_attrs["cma:optimizer"]

[gen740]

I agree with your opinion. However, I think it would be better to ask for another view (since I'm not so familiar with the cmaes sampler).

[c-bata]

OK. Let me remove it in the following pull request.

[c-bata]

Sorry, please ignore this comment since the following test case is incorrect.

There seems not to be enough test case to check this change. For example, I add following scenario, which is failed.

@pytest.mark.parametrize("sampler_opts", [
    {"use_separable_cma": False, "with_margin": False},
    {"use_separable_cma": True, "with_margin": False},
    {"use_separable_cma": False, "with_margin": True},
])
def test_restore_optimizer_from_substrings(sampler_opts: Dict[str, Any]) -> None:
    sampler = optuna.samplers.CmaEsSampler(popsize=8, **sampler_opts)
    optimizer, n_restarts = sampler._restore_optimizer([])
    assert optimizer is None
    assert n_restarts == 0

    def objective(trial: optuna.Trial) -> float:
        x1 = trial.suggest_float("x1", -10, 10)
        x2 = trial.suggest_float("x2", -10, 10)
        return x1 ** 2 + x2 ** 2

    study = optuna.create_study(sampler=sampler)
    study.optimize(objective, 10)
    completed_trials = study.get_trials(states=[TrialState.COMPLETE])

    optimizer, n_restarts = sampler._restore_optimizer(completed_trials)

    assert optimizer is not None
    assert n_restarts == 1
    if sampler._with_margin:
        assert isinstance(optimizer, CMAwM)
    elif sampler._use_separable_cma:
        assert isinstance(optimizer, SepCMA)
    else:
        assert isinstance(optimizer, CMA)

Let me try to add more test scenarios 🙇

[c-bata]

Thank you for your pull request.
Although there seems not to be enough test cases, I added some test scenarios and confirmed that the current logic passed all scenarios.
https://github.com/gen740/optuna/compare/cmaes-sys-attr-key...c-bata:optuna:cmaes-sys-attr-key?expand=1

Let me add the test scenarios at #4233.
LGTM.

[c-bata]

@eukaryo Let me merge this PR after you approved 🙏

[eukaryo]

Sorry for the late reply. LGTM