Fast pareto-front calculation for 2D

[contramundum53]

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.

• I agree this patch may be ported to Goptuna by other Goptuna contributors.

Reference Issues/PRs

Fixes #63.

What does this implement/fix? Explain your changes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (29a5833) 62.88% compared to head (1203501) 68.22%.
Report is 196 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #687      +/-   ##
==========================================
+ Coverage   62.88%   68.22%   +5.33%     
==========================================
  Files          35       35              
  Lines        2250     2329      +79     
==========================================
+ Hits         1415     1589     +174     
+ Misses        835      740      -95     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HideakiImamura]

@not522 @nabenabe0928 Could you review this PR?

[nabenabe0928]

@not522 @nabenabe0928 Could you review this PR?

Sure!

[not522]

This is out of the scope of this PR, but the implementation for dominatedTrials is buggy when all values are same between two trials. They should be non-dominated, but it makes them dominated. Could you fix it?

[not522]

I think the variable and function names sound unnatural. How about the followings?
dominatedTrials -> dominationFlags
getIsDominatedTrial -> evaluateTrialDomination

[contramundum53]

I think renaming dominatedTrials to isDominatedTrials fixes all oddness about the naming. How about that?

[not522]

I think starting a variable name with "is" may not be appropriate for an array. That is, "... is dominated trials" is grammatically incorrect.

[HideakiImamura]

Thanks for the PR. I have several comments. PTAL.

[HideakiImamura]

LGTM.

[contramundum53]

I think starting a variable name with "is" may not be appropriate for an array. That is, "... is dominated trials" is grammatically incorrect.

@not522 How about isDominated and getIsDominated then?

[not522]

How about isDominated and getIsDominated then?

It sounds more natural. 👍

[not522]

This condition is buggy when the values between trials are the same.

[contramundum53]

In that case, both trials should be considered undominated.

[nabenabe0928]

Now given a sequence of 2D vectors $\{(a_n, b_n)\}_{n=1}^N$ such that $a_1 \leq a_2 \leq \dots \leq a_N$ and $b_i \leq b_j$ for all $\{(i, j) \in [N] \times [N] \mid a_i = a_j, i < j\}$ where $[N] \coloneqq \{1,2,\dots,N\}$, the condition here is a bit strange when the equality holds.

For example, $a_i \leq a_j$ holds from the assumption and we now assume that $b_i = b_j$.
As mentioned above, let's assume that $a_i = a_j$ and $b_i = b_j$ do not dominate each other.
However, $a_i < a_j$ and $\min_{n \in [i]} b_i = b_j$ could still hold.
It means that there could exist $(a_i, b_i)$ such that $a_i < a_j$ and $b_i \leq b_j$, which weakly dominates $(a_j, b_j)$.

Btw, I guessed you are trying to get "weak dominance" from getIsDominatedTrialND.

Maybe I am wrong, if so, correct me please!

const normalizedValues = [[0, 1], [1, 1], [2, 1]]

const getIsDominatedTrial2D = (normalizedValues: number[][]) => {
    const sortedValues = normalizedValues
      .map((values, i) => [values[0], values[1], i])
      .sort((a, b) => a[0] !== b[0] ? a[0] - b[0] : a[1] - b[1])
    let minValue1 = sortedValues[0][1]
    const dominatedTrials: boolean[] = new Array(normalizedValues.length).fill(
      true
    )
    sortedValues.forEach((values) => {
      if (values[1] <= minValue1) {
        dominatedTrials[values[2]] = false
        minValue1 = values[1]
      }
    })
    return dominatedTrials
}

console.log(getIsDominatedTrial2D(normalizedValues))

Output

[ false, false, false ]

I guess (if we need weakly dominated trials) what we want is:

[ false, true, true ]

[nabenabe0928]

Quick solution to this is the following:

const normalizedValues = [[0, 1], [1, 1], [2, 1]]

const getIsDominatedTrial2D = (normalizedValues: number[][]) => {
    const sortedValues = normalizedValues
      .map((values, i) => [values[0], values[1], i])
      .sort((a, b) => a[0] !== b[0] ? a[0] - b[0] : a[1] - b[1])
    let minValue1 = sortedValues[0][1]
    let latestParetoSolution = sortedValues[0].slice(0, 2)
    const dominatedTrials: boolean[] = new Array(normalizedValues.length).fill(
      true
    )
    sortedValues.forEach((values) => {
      if (values[1] < minValue1 || latestParetoSolution.every((v, i) => v === values[i])) {
        dominatedTrials[values[2]] = false
        minValue1 = values[1]
        latestParetoSolution = values.slice(0, 2)
      }
    })
    return dominatedTrials
}

console.log(getIsDominatedTrial2D(normalizedValues))

[HideakiImamura]

See https://github.com/optuna/optuna/blob/master/optuna/study/_multi_objective.py#L101. Optuna uses weakly dominates for comparison. It seems that @nabenabe0928 is correct.

[not522]

Thank you for pointing out. My statement is incorrect and @nabenabe0928's one is correct, I think.

[nabenabe0928]

The new modification fails with the following case.

const normalizedValues = [[0, 1], [1, 2], [2, 1], [3, 0], [3, 0]]

We get:

[ false, true, false, false, false ]

But we would like to have:

[ false, true, true, false, false ]

I think the suggestion here is sufficient.

[nabenabe0928]

Btw, it is still necessary.
I will check.

[nabenabe0928]

const values = [[2, 2], [3, 2], [10, 2]]
const sortedValues = values.map((v, i) => [v[0], v[1], i]).sort()
console.log(sortedValues)

Output

[ [ 10, 2, 2 ], [ 2, 2, 0 ], [ 3, 2, 1 ] ]

So you still need to change the comparator inside sort().

Suggested change

	.sort()
	.sort((a, b) => a[0] !== b[0] ? a[0] - b[0] : a[1] - b[1])

Output after the change:

[ [ 2, 2, 0 ], [ 3, 2, 1 ], [ 10, 2, 2 ] ]

[nabenabe0928]

Suggested change

	return normalizedValues.map((value) => value[0] !== best_value)
	return normalizedValues.map((values) => values[0] !== best_value)

[contramundum53]

@nabenabe0928 @not522 Thank you for pointing them out. I fixed the bug. PTAL.

[nabenabe0928]

@contramundum53

I commented below to bundle the discussion in one place!
#687 (comment)

[nabenabe0928]

Or probably, it should work as well.

Suggested change

	minValue1 = values[1]
	}
	maxValue0 = values[0]
	}
	maxValue0 = values[0]
	minValue1 = Math.min(...[minValue1, values[1]])

[nabenabe0928]

I guess it is clearer as the inequality is always removed by the first condition.

Suggested change

	(values[1] >= minValue1 && values[0] > maxValue0)
	(values[1] === minValue1 && values[0] > maxValue0)

[nabenabe0928]

LGTM!

[contramundum53]

@not522 Thank you for your review. I think I fixed the problems, PTAL.

[not522]

Thank you for your update. I confirmed that this PR makes the Pareto-front calculation faster when many trials are best trials. LGTM.