Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify documentation of Augmented Lagrangian initial coefficients settings #197

Open
paulduf opened this issue Mar 25, 2022 · 1 comment

Comments

@paulduf
Copy link

paulduf commented Mar 25, 2022

I have a few comments to make regarding the docstring of AugmentedLagrangian.set_coefficients

1. Error in the docstring

The docstring relates to the following formula for setting the lagrangian parameter:

lam = iqr(f) / (sqrt(n) * iqr(g))

whereas, in the code, I can read

self.lam[idx_inequ] = df / (self.dimension * dG[idx_inequ] + 1e-11 * (df + 1))

It looks like the new default is dimension instead of sqrt(dimension)

2. What does this mean ?

At the end of the docstring I can read:

""""Set lam and mu until a population contains more than 10% infeasible
and more than 10% feasible at the same time. Afterwards, this at least...?.. """.```

But I don't find the corresponding piece of code ?

3.

My last request for documentation is about this code

# take min or max with existing value depending on the sign average
            # we don't know whether this is necessary
            isclose = np.abs(sign_average) <= 0.2
            idx1 = _and(_and(_or(isclose,
                                 _and(_not(self.isequality), sign_average <= 0.2)),
                             _or(self.mu == 0, self.mu > mu_new)),
                             idx)  # only decrease
            idx2 = _and(_and(_and(_not(isclose),
                                  _or(self.isequality, sign_average > 0.2)),
                                  self.mu < mu_new),
                                  idx)  # only increase
            idx3 = _and(idx, _not(_or(idx1, idx2)))  # others
            assert np.sum(_and(idx1, idx2)) == 0
            if np.any(idx1):  # decrease
                iidx1 = self.mu[idx1] > 0
                if np.any(iidx1):
                    self.lam[idx1][iidx1] *= mu_new[idx1][iidx1] / self.mu[idx1][iidx1]
                self.mu[idx1] = mu_new[idx1]
            if np.any(idx2):  # increase
                self.mu[idx2] = mu_new[idx2]
            if np.any(idx3):
                self.mu[idx3] = mu_new[idx3]

Which is hard to read and looks somewhat related to my 2nd point. This looks like this is to decide whether mu_new should be chosen as the new setting if the user has already set values for mu, based on the value of sign_average and other conditions. So does it relate to the quoted part of the docstring and should it be updated ?

@nikohansen
Copy link
Contributor

  1. fixed with the next push.
  2. sign_average < -0.8 means less than 10% are infeasible, but I also suspect that the text is not accurate.
  3. at some point to be reviewed/revised

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants