-
Notifications
You must be signed in to change notification settings - Fork 558
New KNITRO direct solver interface #3707
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
Conversation
611fb3b
to
0d7a162
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3707 +/- ##
==========================================
- Coverage 89.18% 89.16% -0.03%
==========================================
Files 896 906 +10
Lines 103725 104488 +763
==========================================
+ Hits 92508 93166 +658
- Misses 11217 11322 +105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @eminyouskn - okay, so, the tests are running but a large number of them are failing. Here is part of the stacktrace:
Most of the failures are just tolerance problems / the tolerance needs to be loosened. |
One-off failures:
Second one-off:
|
Thanks @mrmundt for the report. I didn’t run these tests locally since I didn’t have NumPy installed, and in that case, the tests are skipped. I’ll make sure to check them before pushing code from now on. I do have a question: is there a way to adjust the tolerance for all these tests at once? Ideally, we’d only change it for Knitro, but I imagine that might be more complicated. Regarding the two remaining failures, I’ve already fixed the one related to the objective. However, I’m not sure how to address the stale one, it’s not clear to me what exactly that test is supposed to check. |
Co-authored-by: Miranda Mundt <[email protected]>
We want this in today's release, so we are going to merge it. However, we think there are some lingering opportunities for improvement that we'd like to address with a future PR. Thanks, @eminyouskn ! |
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.
This is great. Some notes for future development / improvements...
ConfigValue( | ||
domain=Bool, | ||
default=False, | ||
doc="KNITRO solver does not allow variable removal. We can either make the variable a continuous free variable or rebuild the whole model when variable removal is attempted. When `rebuild_model_on_remove_var` is set to True, the model will be rebuilt.", |
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.
It would be good to split this into multiple strings to keep line lengths <= 88 characters
ConfigValue( | ||
domain=Bool, | ||
default=False, | ||
doc="To evaluate non-linear constraints, KNITRO solver sets explicit values on variables. This option controls whether to restore the original variable values after solving.", |
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.
It would be good to split this into multiple strings to keep line lengths <= 88 characters
if item.has_lb(): | ||
bounds_map[BoundType.LO][i] = value(item.lb) | ||
if item.has_ub(): | ||
bounds_map[BoundType.UP][i] = value(item.ub) |
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.
It would be more efficient to do:
if item.has_lb(): | |
bounds_map[BoundType.LO][i] = value(item.lb) | |
if item.has_ub(): | |
bounds_map[BoundType.UP][i] = value(item.ub) | |
lb, ub = item.bounds | |
if lb is not None: | |
bounds_map[BoundType.LO][i] = lb | |
if ub is not None: | |
bounds_map[BoundType.UP][i] = ub |
if item.equality: | ||
bounds_map[BoundType.EQ][i] = value(item.lower) | ||
continue | ||
if item.has_lb(): | ||
bounds_map[BoundType.LO][i] = value(item.lower) | ||
if item.has_ub(): | ||
bounds_map[BoundType.UP][i] = value(item.upper) |
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.
(Note: .lower
returns an expression, .lb
returns the value)
Also, it is more efficient to retrieve the bounds at once:
if item.equality: | |
bounds_map[BoundType.EQ][i] = value(item.lower) | |
continue | |
if item.has_lb(): | |
bounds_map[BoundType.LO][i] = value(item.lower) | |
if item.has_ub(): | |
bounds_map[BoundType.UP][i] = value(item.upper) | |
lb, body, ub = item.to_bounded_expression(evaluate_bounds=True) | |
if item.equality: | |
bounds_map[BoundType.EQ][i] = lb | |
continue | |
if lb is not None: | |
bounds_map[BoundType.LO][i] = lb | |
if ub is not None: | |
bounds_map[BoundType.UP][i] = ub |
return knitro.KN_set_double_param | ||
elif param_type == knitro.KN_PARAMTYPE_STRING: | ||
return knitro.KN_set_char_param | ||
raise DeveloperError() |
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.
This is great defensive programming, but it would be good for these exceptions to have informative messages to help us track things down if they are ever reported.
|
||
""" | ||
generator = block.component_data_objects( | ||
Constraint, descend_into=True, active=True, sort=True |
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.
It will be significantly more efficient to not use sort=True
and instead use sort=SortComponents.deterministic
This method computes the gradient of the expression with respect to all | ||
variables and stores the results in the grad attribute. | ||
""" | ||
derivative = reverse_sd(self.func_expr) |
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.
TODO: we should evaluate the computational performance of native python symbolic derivatives. Some alternatives:
- compile the expressions into executable python code (to avoud needing to use the Pyomo expression evaluator
- use AD instead of SD (particularly the APPSI c-extension?)
Fixes NA
Summary/Motivation:
This PR introduced a new KNITRO direct solver interface.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: