-
Notifications
You must be signed in to change notification settings - Fork 277
Enhance getSolution(): convert lists to NumPy arrays #2603
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
base: latest
Are you sure you want to change the base?
Enhance getSolution(): convert lists to NumPy arrays #2603
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## latest #2603 +/- ##
==========================================
- Coverage 81.06% 81.06% -0.01%
==========================================
Files 347 347
Lines 85239 85219 -20
==========================================
- Hits 69100 69082 -18
+ Misses 16139 16137 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
What do you reckon @mathgeekcoder ? |
|
@jajhall: I wouldn't accept this change. I have thought about returning these as numpy arrays (instead of lists), but this is not the best way to do this - it will cause an unnecessary copy and breaks the typing information. @ParthPore10: The code isn't bad, but could be better. Per the documentation, That said, if we modify py::class_<HighsLp>(m, "HighsLp", py::module_local())
.def(py::init<>())
.def_property("col_cost_", make_readonly_ptr(&HighsLp::col_cost_),
make_setter_ptr(&HighsLp::col_cost_))This should avoid unnecessary copies and return a numpy array on the python side. If this is changed, the corresponding types in class HighsSolution:
col_dual: list[float] # list should be changed to numpy.ndarray[typing.Any, numpy.dtype[numpy.float64]]
col_value: list[float]
dual_valid: bool
row_dual: list[float]
row_value: list[float]
value_valid: bool
def __init__(self) -> None: ...I'm hesitant to use numpy arrays everywhere (in some cases lists are actually faster), but for this it should be fine. BTW: if you use the |
Thanks, I guess that returning these as numpy arrays (instead of lists) means that we avoid users having to avoid this issue? |
jajhall
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.
@ParthPore10 Please edit the PR as suggested by @mathgeekcoder
Great question! I didn't think it would help - but I gave it a quick test, and you are correct - that issue doesn't seem to occur with a numpy array from pybind (using my suggested change above). e.g.,: lp = h.getLp()
# fast (uses numpy)
value = [lp.col_cost_[icol] for icol in range(num_vars)]
# slow (uses list)
value = [lp.col_lower_[icol] for icol in range(num_vars)] |
@ParthPore10: Since this is a larger change, and modifying the C++ code - please let me know if you'd prefer me to make these changes, or if you'd like to make an attempt. |
|
Hii @mathgeekcoder , @jajhall tthanks for pointing that out. I’ll review the problem and take another crack at it. |
d87bb3f to
abf026e
Compare
|
@ParthPore10: there's something odd about your PR. The code-diff looks like it's comparing to an older branch, e.g., it doesn't recognize the changes for Also, BTW: I wouldn't change the bazel build; there's already a setter function (so you should use that instead of writing your own); and you didn't actually change the getter/setter for |
This PR updates the Python interface of getSolution() to return NumPy arrays instead of native Python lists for the following attributes:
Returning NumPy arrays improves numerical performance and consistency, since most users already work with NumPy-based data when constructing models or performing post-processing. This change avoids redundant np.array() conversions in downstream applications and aligns input and output data types.
Implementation Details
Overrides getSolution() to convert solution lists into NumPy float arrays using _as_float_array().
Ensures compatibility by preserving the original structure of the solution object.
References
Fixes #1590