artofscience / SAOR

Sequential Approximate Optimization Repository
GNU General Public License v3.0
5 stars 1 forks source link

Restructuring subproblem/approximation/intervening #57

Closed artofscience closed 3 years ago

artofscience commented 3 years ago

Please consider the restructuring of problem, approx, int. var.

@Giannis1993 Can you look at SphericalTaylor2, NonSphericalTaylor2 and make them in line with Taylor1/Taylor 2? Also can you fix the test_taylor.py file?

This pr also includes removal of dxdy and ddxddy (test work)

artofscience commented 3 years ago

Some notes on some findings during this PR:

With the new limits strategies, I suppose we can remove xmin and xmax from proble.py? @Giannis1993 @MaxvdKolk @aatmdelissen

In subproblem.py the limit strategy "NoMoveLimit" is vague. name should be altered.

@aatmdelissen you noted subproblem.py does not require to hold (n,m), since problem does not need it. Only issue is, what if dg is of different size between design iterations, doe you want to detect this?

@Giannis1993 can you fix mixed.py (contains calls to intervening functions)

@Giannis1993 test_intervening requires more tests.

aatmdelissen commented 3 years ago

Some notes on some findings during this PR:

With the new limits strategies, I suppose we can remove xmin and xmax from proble.py? @Giannis1993 @MaxvdKolk @aatmdelissen

I think the global bounds need to remain in Problem for now, since they are part of the problem definition. They might be different from the bounds on the SubProblem.

@aatmdelissen you noted subproblem.py does not require to hold (n,m), since problem does not need it. Only issue is, what if dg is of different size between design iterations, doe you want to detect this?

A change in size does not need to be supported, but we can do an assert and check the sizes every subsolve.

self.m, self.n = -1, -1
if self.m < 0 and self.n<0:
    self.m, self.n = dg.shape
else:
    assert(self.m, self.n == dg.shape)
MaxvdKolk commented 3 years ago

I think the global bounds need to remain in Problem for now, since they are part of the problem definition. They might be different from the bounds on the SubProblem.

Might be that we can store them inside Problem as one of these Movelimit-like classes, rather than explicit as xmin, xmax? In that case, we could directly clip the domains using the clip functions from the move limit. The name of the class might be a bit confusing though, so maybe xmin,xmax is the simplest for now.

A change in size does not need to be supported, but we can do an assert and check the sizes every subsolve.

This mostly refers to changing number of constraints (as in those vanishing formulations)? Probably better to pick that up in a separate PR once this restructuring is merged

artofscience commented 3 years ago

Gentlemen, what about accepting this PR and make new PR's for the mixed scheme / gcmma?

MaxvdKolk commented 3 years ago

Gentlemen, what about accepting this PR and make new PR's for the mixed scheme / gcmma?

Yes, let's do that. That would make reviewing / contributing to those changes easier since it would be separate from the set of changes in this PR. 👍