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

Inconsistency between RH and dsdp updates #53

Open
Robbybp opened this issue Mar 8, 2021 · 7 comments
Open

Inconsistency between RH and dsdp updates #53

Robbybp opened this issue Mar 8, 2021 · 7 comments
Assignees

Comments

@Robbybp
Copy link
Contributor

Robbybp commented Mar 8, 2021

double ALPHA = 1.0;

In dot_driver, RH mode (npdp_strategy) performs the sensitivity update with ALPHA = -1.0, whereas dsdp mode performs the update with ALPHA = 1.0. How the code is currently written, I believe ALPHA = -1.0 is correct for both cases. This way the correct update will occur with

DeltaP = perturbed_value - original_value

in both cases. As currently written, the above is only correct for RH mode. DeltaP must have the opposite sign as above for dsdp mode. The reason we need this minus sign (ALPHA = -1.0) is that in k_aug, we calculate and store the negative sensitivity matrix. I propose we fix this by calculating the positive sensitivity matrix and switching ALPHA to positive one for both modes. This way perturbed - original can be used for both updates, which I think is logical.

Let me know if I have this wrong. What I'm saying here is based on my experience getting tests to pass in Pyomo/pyomo#1613.

@dthierry
Copy link
Owner

dthierry commented Mar 8, 2021

I think you are right. I vaguely remember that there was actually a great reason for have ALPHA=1.0, but I can't remember it now.

Maybe we need to put that as a comment in the dot_driver.c and print a warning saying that this has changed now, because the MHE people are assuming that ALPHA = 1.0.

@Robbybp
Copy link
Contributor Author

Robbybp commented Mar 9, 2021

Yeah printing a warning is probably the right call. Do you know off the top of your head if the MHE people are using the opposite perturbation (original - perturbed) or the opposite constraint (param - var == 0)? I still want to take derivatives wrt sensitivity variables via ASL so we don't have to rely on the form of the constraint expression.

@Robbybp
Copy link
Contributor Author

Robbybp commented Mar 9, 2021

Also what do you think about storing the positive sensitivity matrix?

@dthierry
Copy link
Owner

dthierry commented Mar 9, 2021

They are doing original - perturbed.
I think you can do that, if you declare the vector of p as variables and then pass their positions to k_aug with the suffixes.
So assuming p appears linearly:
You need to partition grad_c into [gradx_c^T gradp_c^T]^T, then permute either gradx_c or gradp_c so they match the positions of c (with suffixes).
Then assemble the new K:

[hessXX L, gradx_c
grad_c^T, 0] =

[0^T, gradp_c^T]^T

If you have a nonlinear form of p in c of f though, you need to do this also with hess_L:=
[hess_XX, hess_XP,
hess_PX, hess_PP]
so you can extract the blocks that you want (hess_XX and hess_PX), I suppose you hace to do this anyway for the linear in p case.

So to summarise, you could use suffix magic to partition and re-assemble.

You could even partition K into small [K_i, ....] and do some decomposition stuff....

@dthierry
Copy link
Owner

dthierry commented Mar 9, 2021

Also what do you think about storing the positive sensitivity matrix?

What do you mean?

@Robbybp
Copy link
Contributor Author

Robbybp commented Mar 9, 2021

Yeah that's what I was thinking about for the variables - you'll just need to know which variables are sens vars, instead of which constraints.

Right now I think the matrix that is stored in dsdp_in_.in, assuming the constraint is written var - param == 0, is K^-1 R. The sensitivity matrix according to the implicit function theorem is -K^-1 R. So when I say "positive sensitivity matrix," I mean this latter quantity. If we stored this matrix, we would perform a "perturbed minus original" update with ALPHA=1.0, which I think makes more sense.

@dthierry
Copy link
Owner

dthierry commented Mar 9, 2021

I see. I agree, we should save that matrix instead.

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