Bug in python callbacks user_solution and mip_solution

[hlinsen]

• to_array accessor of readonly_ptr_wrapper returns wrong values after the first one.

and solution[0] = 14.0
and solution[1] = 118.0
to_array(2) = [14.0, 1.7991808152773074e-307]

see https://github.com/ERGO-Code/HiGHS/blob/latest/examples/call_highs_from_python.py#L258

• user_solution is not fully integrated at the python layer https://github.com/ERGO-Code/HiGHS/blob/latest/src/highspy/_core/cb.pyi#L28

[jajhall]

• to_array accessor of readonly_ptr_wrapper returns wrong values after the first one.

and solution[0] = 14.0
and solution[1] = 118.0
to_array(2) = [14.0, 1.7991808152773074e-307]

see https://github.com/ERGO-Code/HiGHS/blob/latest/examples/call_highs_from_python.py#L258

I've added MIP callback tests in examples/call_highs_from_python.py, in the branch https://github.com/ERGO-Code/HiGHS/tree/fix-2270 but I don't have the Python skills to reproduce the issue.

Can you have a look at this and the other part of the issue, @mathgeekcoder?

[mathgeekcoder]

FYI: I can reproduce, and I think I might know the fix. Testing now... Having some issues with my dev environment, so might take a little while to verify.

[mathgeekcoder]

@jajhall - the issue with readonly_ptr_wrapper is an easy (one line) fix, however the whole premise behind readonly_ptr_wrapper is a bit of a hack that becomes more obvious when trying to support user_solution and cut_pool.

Basically, since the callbacks are C style, there's some potential issues with memory safety. I have a working solution to clean it all up... but has a minor breaking changes to the C API and highspy (removing to_array since no longer needed).

I'll push a PR to your fix2270 branch, but happy to discuss/modify if you want to avoid breaking changes.

[jajhall]

Basically, since the callbacks are C style, there's some potential issues with memory safety. I have a working solution to clean it all up... but has a minor breaking changes to the C API and highspy (removing to_array since no longer needed).

Thanks. I wrote the callbacks C-style because it made it simpler to add them to the C API. Clearly it wasn't the best idea....

[mathgeekcoder]

Thanks. I wrote the callbacks C-style because it made it simpler to add them to the C API. Clearly it wasn't the best idea....

I can appreciate the idea behind it. It's a design choice with its own pros/cons. Technically, it should be possible to use that C-style safely, but moving "ownership" of the memory will just make it easier for users to avoid issues.

@hlinsen I hope to have an updated PR before end of the week, but in the meantime, I suggest not to use the to_array function. It's a little clunky anyhow, and there's several better alternatives in highspy. With any luck I'll be able to deprecate that function.

[hlinsen]

Thanks. I wrote the callbacks C-style because it made it simpler to add them to the C API. Clearly it wasn't the best idea....

I can appreciate the idea behind it. It's a design choice with its own pros/cons. Technically, it should be possible to use that C-style safely, but moving "ownership" of the memory will just make it easier for users to avoid issues.

@hlinsen I hope to have an updated PR before end of the week, but in the meantime, I suggest not to use the to_array function. It's a little clunky anyhow, and there's several better alternatives in highspy. With any luck I'll be able to deprecate that function.

Thanks @mathgeekcoder! For the second part of the issue, can I pull the changes now and set a user_solution through python?

[mathgeekcoder]

Thanks @mathgeekcoder! For the second part of the issue, can I pull the changes now and set a user_solution through python?

No, sorry - not yet. I'm almost done, adding some extra tests in the C/C++/Python code. On track to update the PR on Monday.

[mathgeekcoder]

@hlinsen I've updated my branch with changes. The PR review process may require further changes, and the official build will likely take a while to include this fix. But if you're desperate to experiment, feel free to play with my branch - however, caveat emptor.

[jajhall]

The changes made in #2278 appear to have triggered #2400

Perhaps we need more general ways of getting the components of data_out @mathgeekcoder?

Since @odow will also be in Edinburgh in a couple of weeks, maybe that's a good time to discuss this?

[odow]

:+1: