Enhance getSolution(): convert lists to NumPy arrays#2603
Enhance getSolution(): convert lists to NumPy arrays#2603ParthPore10 wants to merge 3 commits intoERGO-Code:latestfrom
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.
@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