-
Notifications
You must be signed in to change notification settings - Fork 38
ACOPF polar branch #176
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
ACOPF polar branch #176
Conversation
, solver parameters, and unit tests
|
Looks like I can't edit the reviewer tab, @simonbowly can you please assign yourself? |
|
I will start working on the docs and API once this is merged. |
JaromilNajman
left a comment
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.
I have a few comments and questions.
|
Once this branch is merged, I will take care of the docs. I see that you already adjusted the API and tests. |
|
Thanks for reviewing the code @JaromilNajman! If you're happy I'm happy. For the docs I guess there is not much to do, unless you wanted to add more details about the polar formulation? Other than that it's just mentioning the new |
I wanted to add a subsection in https://gurobi-optimods.readthedocs.io/en/stable/mods/opf/opf_specification.html about "Polar Formulation". Nothing big, just to have it complete. If we want polar to be the new default, then we need to adjust the docs as well, since the values for the example we provide will change slightly.
What do you mean? Wouldn't we have to explain the new settings in |
|
@simonbowly Could you please add me as a reviewer? Then you could accept the changes and I can keep reviewing. This way the branch would be ready to merge once I am happy :) |
Ah yes, maybe there's more than I thought. I'll leave it to you.
Yep, no need to wait for me. You don't need to be a reviewer to add comments, and reviews don't block anything on this repo anyway. The easiest way may be for Hassan to give you write access to his fork, then you can just push to this branch. Or, you could prepare the docs changes on a different branch with a separate PR. |
JaromilNajman
left a comment
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.
Two minor comments. Looks good otherwise.
|
@simonbowly I think we're ready to merge this one now. |
|
@hhijazi or @JaromilNajman I ran into a bug when testing AC with branch switching (some tests were skipped, they're now re-enabled). Could you please review the fixes in 0e61518? |
|
This should be the correct fix. Let's see what @hhijazi says. |
|
Looks good to me, thanks Simon! |
|
Great, merged! @JaromilNajman please go ahead with any docs changes needed |
|
Thanks for your great work on this @hhijazi :-) |
Description
New acopf polar branch using nlfuncs