-
Notifications
You must be signed in to change notification settings - Fork 25
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
Fix constraints and objective output #76
Conversation
@johnjasa, one test I'd like to do to feel perfectly comfortable with the constraints is the prob.check_totals() from OpenMDAO to see if the gradients of the constraints are working. Unfortunately, I am unable to get that to work as it gets stuck during the prob.run_model() step. Any suggestions? Edit: nevermind I just had too many turbines which made run_model take forever. Reducing the test to 2 turbines resulted in the desired output |
Nice, great resolution, @jefalon! Did the computed gradients match the FD well? |
not exactly:
The gradient of the objective function seem right, but the power constraints seem very wrong. That said, it seems like OpenMDAO is only using a 1e-6 perturbation.
Since these are in meters, that is like shifting the turbine by a micrometer and expecting a good response. Seem off |
Oh yes, those are quite wrong for the constraint, the top section. Is that with the power constraint, not the spacing constraint? I think we have more digging to do. There's a chance the FD is wrong, but I wouldn't expect the computed gradients to be of opposite sign like we're seeing. |
I just updated my comment to include more information. Additionally, the min_dist constraint seems to work flawlessly (I just turned it off to emphasize the power constraint) |
Right on, I hear what you're saying about the step size. We could try a few different step sizes to see if the computed power constraint is correct. I guess what we're stepping towards is that the power objective has correct gradients, right? But the power constraint doesn't. So we need to find out the implementation difference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great fixes here, Jeff! All good to merge on my end.
Purpose
Fixes two bugs. First, by splitting up_k during the update step of ControlUpdater(), u_k and p_k update allowing for computation of objective values and constraints during optimization. Second, replacing assignment of control to creation of new Constants() in WindFarmManager restore taylor_test convergence of order 2.
Type of change
Select the appropriate type(s) that describe this PR
Testing
In order to properly test the Taylor test, we would need to setup a regression test for each objective. That might be too much.
Checklist
Put an
x
in the boxes that apply.