Conversation
| BLXAlphaCrossover(), | ||
| SPXCrossover(), | ||
| SBXCrossover(), | ||
| VSBXCrossover(), |
There was a problem hiding this comment.
Removed because even if the parent has the same variables, the child is created in its neighborhood and the purpose of this test, to be the same individual as the parent, is not achieved.
|
@sawa3030 Could you review this PR? (cc @HideakiImamura ) |
| uniform_crossover_prob: float = 0.5, | ||
| use_child_gene_prob: float = 0.5, | ||
| ) -> None: | ||
| self._eta = eta |
There was a problem hiding this comment.
Is the valid range for
beta_1 = np.power(1 / (2 * us), 1 / (eta + 1))
~~^~~~~~~~~~~
ZeroDivisionError: division by zeroThere was a problem hiding this comment.
I could not find a description of the range of eta in the references in the document, but the following ones mention that it is a non-negative value.
Add a check for non-negative values, since there is also the issue of zero divisions.
Self-adaptive simulated binary crossover for real-parameter optimization
I will implement the same for the SBX as a another PR.
| ) | ||
|
|
||
| # vSBX applies crossover with establishment 0.5, and with probability 0.5, | ||
| beta_1 = np.power(1 / (2 * us), 1 / (eta + 1)) |
There was a problem hiding this comment.
Although it is a very small probability, there is a chance of becoming the denominator in 1 / (2 * us) can be zero since rng.rand()
The below might be safer. (This pseudocode might require modification since us is vector whereas eps is scalar.)
beta_1 = np.power(1 / max((2 * us), eps), 1 / (eta + 1))There was a problem hiding this comment.
There is no strong reason for the eps, but I have chosen to use the same values used in the SBX.
Although out of the range of values actually generated(rng.rand()∈[0,1)), 1 is set as us value in the test. Since zero divisions occur even in beta_2, a similar correction has been made.
beta_2 = np.power(1 / (2 * (1 - us)), 1 / (eta + 1))
| ( | ||
| VSBXCrossover(), | ||
| 0.0, | ||
| np.array([-1076.02679423, -2151.84728898]), |
There was a problem hiding this comment.
Question: Where do these values come from? I have no idea about what is tested here.
There was a problem hiding this comment.
Based on #6033 (comment), I modified the following so that zero divisions do not occur.
beta_1 = np.power(1 / np.maximum((2 * us), eps), 1 / (eta + 1))
The original test was [-inf, -inf] because of the zero division, but has been corrected as above.
However, beta_1 still be a very large value, so vsbx generate a child on the -inf side. The value tested here is considered to be a result that depends on the eps value.
There was a problem hiding this comment.
I followed the original test with a random value of 0 and adjusted the test results, but as you pointed out, this is not a very meaningful test, so it may be more appropriate to specify a different meaningful random value.
There was a problem hiding this comment.
I think it's important to keep the test with us = 0 since it covers an edge case. That said, I agree that the test is ambiguous. How about adding a comment to clarify the behavior, as you mentioned here?
There was a problem hiding this comment.
I added a comment on the test content.
sawa3030
left a comment
There was a problem hiding this comment.
Thank you for the update. LGTM!
Motivation
Improve vSBX based on #6000, #6008
Description of the changes
I have changed the implementation to make it possible to create a distribution of child individuals as in the reference paper.
See #6000 for details.
In order to align the paper with the results, the implementation has been changed and the crossover results are no longer compatible with the previous results. Therefore, the test is being rewritten.
I understand that this is not desirable for Optuna.
On the other hand, the fact that the implementation does not match the reference paper is considered a bug, so I create this PR.
Any comments on what form it should take would be appreciated.