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

add multi-run and variable scaling #354

Closed
wants to merge 0 commits into from

Conversation

Osburg
Copy link
Collaborator

@Osburg Osburg commented Feb 22, 2024

This PR is a draft for an implementation of multi-start doe as described in #343 and for optional scaling of decision variables to (-1,1). Closes #343 and #351.

@jduerholt
Copy link
Contributor

Hi @Osburg, thank you very much. I try to have a look over the weekend. Best, Johannes

@jduerholt
Copy link
Contributor

Hi @Osburg

I walked a bit through the DoE code and understood thereby also what was wrong with the gradients with my very simple implementation. Doing this, I had an idea which perhaps would yield a more easy solution:

What you are doing is to transform the problem aka the domain object to use bounds between -1 and 1 for the continuous inputs. Doing this, you also have to transform the constraints which is easy for the linear ones and more complex for the nonlinears and would also require changes on the code base for the NChooseK constraints, so you are transforming the complete optimization problem to bounds -1 and 1.

My idea would be to keep the actual optimization problem (solved by ipopt) in the original domain but calculate the actual objective in the scaled one, this would imply the following steps for a given X and the D-optimality criterion:

  1. Scale the sample vector X between -1 and 1 and receive X_s.
  2. Compute the model matrix X_m out of X_s based on the model
  3. Compute logdet(X_m.T@X_m)

Regarding the jacobian, you get the outer one based one by torch autodiff and the inner one of X_m.T@X_m with respect to X_s by the help of symbolic differentiaion by formulaic and sympy. What would be currently missing is just the jacobian of X_s with respect to X which should be relatively easy to calculate or, via the chain rule it could then be combined with the rest.

What do you think? For me this sounds easier and more elegant as we get rid of the transformation of the domain.

Best,

Johannes

@Osburg
Copy link
Collaborator Author

Osburg commented Feb 23, 2024

Hi @jduerholt,

thanks for having a look at this PR. I think you are right, scaling inside the objective is easier. We could give the Objective class another member containing information about the trafo, change Objective.evaluate() to sth like

def evaluate(x):
    return _evaluate(self.trafo(x))

_evaluate() would be a new abstract method defined just as evaluate() is defined now. Same could be done with the gradient, the only difference is that we would have to additionally scale the result by some constant factors (the derivative of the trafo should be some const. diagonal matrix, right?). This way we fix the issue for all current and future objective classes. What do you think about this?

  • what do you say about keeping sth like the class DoE and generate stuff like the scaling trafo (and/or additional stuff that is unneccessarily done in find_local_max_ipopt() ) in there? find_local_max_ipopt() is already so bloated (and i find design.py very messy in general :D Think I should tidy this up somewhen...), would like to avoid putting more stuff in this function...

Cheers
Aaron :)

@jduerholt
Copy link
Contributor

Hi @Osburg,

fine for me with the new abstract method evaluate. For the jacobian I would then setup something like:

def evaluate_jacobian(self, x):
    return self._evaluate_jacobian(x) * self.trafo_jacobian()

As the new jacobian is just the old one multiplied with a constant matrix which just depends on the lower and upper bounds of the features and the lower and upper bound of the scaling operation, which was in this discussion always [-1,1].

One could think about setting up the transformation as an own class which one provides with the lower and upper bound in which the scaling should happen.

This would also be my preferred solution.

In general, I like the idea of structuring the code better but I would not introduce a new class DoE but would rather go for a tighter integration into bofire by creating an abstract functional class from which then the actual DoEStrategy and SpaceFillingStrategy would inherit. What do you think?

Can you estimate, if you have time this week for a first working implementation of the scaling? I have a use case in which I want to try it, if not, I will give it a try for a first version.

Best,

Johannes

@jduerholt jduerholt mentioned this pull request Feb 26, 2024
@Osburg
Copy link
Collaborator Author

Osburg commented Feb 28, 2024

Hi @jduerholt ,

yes, will have time to do work on it no later than this weekend. + thanks for starting in #358.

Cheers
Aaron :)

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

Successfully merging this pull request may close these issues.

Scaling in DoE
2 participants