Skip to content
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

ENH: Add callbacks to highspy, use std::function #1447

Merged
merged 14 commits into from
Oct 31, 2023

Conversation

HaoZeke
Copy link
Contributor

@HaoZeke HaoZeke commented Oct 8, 2023

This also updates the callback mechanism to use std::function, as it interfaces a lot better with native Python functions. Since it is logically separate from #1405, and required changes to the existing HiGHs API, I think it should be reviewed separately.

Closes #911

@HaoZeke HaoZeke changed the title ENH: Add callbacks to highspy ENH: Add callbacks to highspy, use std::function Oct 8, 2023
Copy link
Sponsor Member

@jajhall jajhall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for tidying up some code that I was not really qualified to write, but was needed by a major user.

I have a reservation and a question

  • Changing the user call-back function from void (*user_callback) to HighsCallbackFunctionType, where HighsCallbackFunctionType is

std::function<void(int, const std::string&, const HighsCallbackDataOut*, HighsCallbackDataIn*, void*)>)

breaks the API that's been defined in 1.6.0 - surely. I've already announced the new call-back definition at a workshop, and at least two major users are writing callbacks on this basis. I guess that we can communicate with them - they are "friends" - rather than postpone your modifications to the release of HiGHS 2.0. I'm inclined to do this, as the advance that you've made - in cleaning-up the call-backs code and extending call-backs to the Python API - is worth the trouble.

  • It looks to me as if the use of the unwieldy std::function<void(int, const std::string&, const HighsCallbackDataOut*, HighsCallbackDataIn*, void*)> in check/TestCallbacks.cpp can be replaced by HighsCallbackFunctionType

@HaoZeke
Copy link
Contributor Author

HaoZeke commented Oct 8, 2023

Many thanks for tidying up some code that I was not really qualified to write, but was needed by a major user.

I think it is a well thought out design with very clear test cases (which are really the most important, these sort of second order refactors are easy when test suites guarantee correctness :) )

I have a reservation and a question

  • Changing the user call-back function from void (*user_callback) to HighsCallbackFunctionType, where HighsCallbackFunctionType is

std::function<void(int, const std::string&, const HighsCallbackDataOut*, HighsCallbackDataIn*, void*)>)

breaks the API that's been defined in 1.6.0 - surely.

Not as badly as it might, since for the C-API compatibility there's also HighsCCallbackType c_callback = nullptr;, where HighsCCallbackType is the older C-compatible void* type. This can be used with Highs::setCallback (because of function overloads), as is done for the C API.

I've already announced the new call-back definition at a workshop, and at least two major users are writing callbacks on this basis. I guess that we can communicate with them - they are "friends" - rather than postpone your modifications to the release of HiGHS 2.0. I'm inclined to do this, as the advance that you've made - in cleaning-up the call-backs code and extending call-backs to the Python API - is worth the trouble.

That would be great. In terms of compatibility, if users are not able / willing to make the switch to the new type, they can replace user_callback with c_callback and if they are only using function pointers, things should be OK. In fact, as long as they use the API (Highs::setCallback) instead of populating the HighsCallback struct by hand (i.e. manually setting user_callback), everything should be backwards compatible.

All things considered equal, users of the C++ API will likely prefer to use user_callback with the new HighsCallbackFunctionType simply because it allows for more callable objects (e.g. lambdas, both with and without capture; or operator() objects). The lambda functionality is especially important for efficient Pybind11 (and is a requirement for highspy / scipy users).

  • It looks to me as if the use of the unwieldy std::function<void(int, const std::string&, const HighsCallbackDataOut*, HighsCallbackDataIn*, void*)> in check/TestCallbacks.cpp can be replaced by HighsCallbackFunctionType

Yup, good catch, updated.

This PR is almost ready to go from the highspy end as well, will take it out of draft ASAP.

@jajhall
Copy link
Sponsor Member

jajhall commented Oct 8, 2023

Many thanks for tidying up some code that I was not really qualified to write, but was needed by a major user.

I think it is a well thought out design with very clear test cases (which are really the most important, these sort of second order refactors are easy when test suites guarantee correctness :) )

Thanks: that's a level I understand, and at which I feel I operate reasonably well. It's just the detailed knowledge of C/C++ that I don't have , since I learned it "on the job" after jumping from Fortran 77 at the age of 53!

@HaoZeke HaoZeke force-pushed the highspy_callback branch 2 times, most recently from d3a7477 to 58cc6c2 Compare October 29, 2023 18:56
@HaoZeke HaoZeke marked this pull request as ready for review October 29, 2023 18:57
@HaoZeke
Copy link
Contributor Author

HaoZeke commented Oct 29, 2023

Since the SciPy callbacks require some more discussion (here and at SciPy) I think this should be ready to go in as is, @jajhall, it improves (cleans up) the implementation in HiGHs and enables callbacks for highspy.

@jajhall jajhall merged commit 20fc033 into ERGO-Code:latest Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants