ESE algorithm has a comparison that is always true

[neal-ks]

https://github.com/SMTorg/smt/blob/411fdd8a66ba1aa9d6bbd9cdff8d5d3b6a6cc126/smt/sampling_methods/lhs.py#L214C1-L221C32

             if PhiP_best - PhiP_oldbest < tol:
                # flag_imp = 1
                if p_accpt >= 0.1 and p_imp < p_accpt:
                    T = 0.8 * T
                elif p_accpt >= 0.1 and p_imp == p_accpt:
                    pass
                else:
                    T = T / 0.8

This comparison on line 214 looks a bit off. It compares the original best PhiP score at the start of the outer loop iteration to the best score found in the inner loop. The PhiP score is initally based on X_oldbest and is only updated when the score decreases.

                    # Best plan retained
                    if PhiP_ < PhiP_best:
                        X_best = X_
                        PhiP_best = PhiP_
                        n_imp = n_imp + 1

Therefore PhiP_best - PhiP_oldbest should always be <= 0, but the tolerance is hard-coded positive so the condition for the if statement is always true.

I checked in the original paper (https://openturns.github.io/openturns/papers/jin2005.pdf) and I think the comparison should be like this.

if PhiP_oldbest - PhiP_best > tol:

You should be able to reproduce this behavior using this example

from smt.sampling_methods import LHS
import numpy as np

if __name__ == '__main__':
    xlims = np.array([np.zeros(10), np.ones(10)]).T
    sampling = LHS(xlimits=xlims, criterion='ese', random_state=0)
    samples = sampling(50)

[relf]

Very good catch! Thanks for pointing it out! Indeed the condition is wrong, it should have been with a < -tol or as you stated and stated in the paper: PhiP_oldbest - PhiP_best > tol.
This bug is pretty old, as old as SMT, I would say! I am not sure of the impact, the fix may break other tests in SMT as optimized LHS is largely used.

Do you want to submit a PR?

[neal-ks]

I'm happy to put in a PR.