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

Add Python binding #52

Closed
wants to merge 4 commits into from
Closed

Add Python binding #52

wants to merge 4 commits into from

Conversation

jschueller
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@ragonneau ragonneau left a comment

Choose a reason for hiding this comment

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

Hi @jschueller,

Thank you very much for this PR. This is a great work.

I put some comments in the files, they sometimes refer to different lines in the files. Let me know if some are not clear.

Cheers,
Tom.

CMakeLists.txt Show resolved Hide resolved
python/CMakeLists.txt Show resolved Hide resolved
python/prima.py Show resolved Hide resolved
python/prima.py Outdated Show resolved Hide resolved
python/prima.py Show resolved Hide resolved
python/prima.py Outdated Show resolved Hide resolved
python/prima.py Outdated Show resolved Hide resolved
python/prima.py Outdated Show resolved Hide resolved
python/prima.py Outdated Show resolved Hide resolved
python/prima.py Outdated Show resolved Hide resolved
@zaikunzhang
Copy link
Member

zaikunzhang commented Sep 12, 2023

Hi @jschueller ,

Thank you for your immense efforts again!

I am totally ignorant about Python. So I could only give very rough comments, which may very likely be wrong. I am open to discussions.

  1. The first and foremost goal of the Python binding should be the integration of PRIMA solvers to SciPy. So we have to respect the coding style and rules of the SciPy community, and keep communicating with them. I will invite them to this conversation.

  2. For the moment, the first thing on my mind is the building system. According to what I read and my limited understanding, SciPy is adopting Meson (see also comments from the Fortran community). Let us follow.

  3. Avoid the three-language issue. Even though the C interface is extremely nice, we should not use it as a bridge between Fortran and Python (or any other languages). The binding should be directly between Fortran and Python (possibly based on the iso_c_binding subroutines under https://github.com/libprima/prima/tree/main/c). There are many reasons, the following being some examples.

    • We should keep things decoupled unless coupling is inevitable;
    • The complexity of maintenance and debugging increases dramatically with the number of languages involved;
    • The C interface requires a C compiler, but the less requirement the better;
    • It hurts the performance to pass the data through multiple layers of interfaces (this is the least reason in the context of derivative-free optimization).

Many thanks for everything!

Also thank Tom @ragonneau for looking into this PR. Our opinions are similar in most cases thanks to our long-term collaboration.

Best regards,
Zaikun

@jschueller
Copy link
Collaborator Author

jschueller commented Sep 12, 2023

  1. agreed, it is only there waiting for a validated pure Python implementation
  2. the python binding is not compiled, cmake is just used to copy prima.py and run the examples
  3. to use the fortran libs directly the iso_c_binding part should be moved inside the fortran lib, but I dont think it would affect the performance nor the maintenance since the c binding would probably have to keep in sync with the fortran side anyways, as for performance its only for IO, so I dont think it matters

@zaikunzhang
Copy link
Member

zaikunzhang commented Sep 12, 2023

The first and foremost goal of the Python binding should be scipy/scipy#18118. So we have to respect the coding style and rules of the SciPy community, and keep communicating with them. I will invite them to this conversation.

  1. agreed, it is only there waiting for a validated pure Python implementation

Sorry, I do not think we are talking about the same thing --- we are talking about the opposite.

The first and foremost goal of the Python binding should be integrating the Fortran version of
PRIMA into SciPy
.

Yes, there will be eventually a Python implementation, but I do foresee a mature Python implementation will be possible in the near future (e.g., within 2 years, if not longer) given the high complexity of these algorithms. However, the community should not continue to use the buggy F77 version for such a long time.

I have worked on these solvers in all the details, so I know how hard they are. As a reference, Tom has been developing a solver of the same kind named COBYQA for exactly two years (the first commit was on 2 Sept 2021), full time. Tom is quite capable and diligent, but I do not think COBYQA is mature yet (sorry Tom, it is close).

In addition, a new implementation should go through extensive verification and tests before it is safe to use in production (recall how many users SciPy has), which will take time.

That said, the Fortran-Python binding is quite important. It is not something temporary. For the interest of the SciPy community, the pure Python implementation will have to achieve the same performance and robustness as the modern Fortran version before it takes the place of the latter.

Thanks.

@zaikunzhang
Copy link
Member

zaikunzhang commented Sep 12, 2023

the python binding is not compiled, cmake is just used to copy prima.py and run the examples

Probably we are not talking about the same thing again. What I mean is the building system for compiling the Fortran code and making it callable in Python. The SciPy community has announced that they will move to Meson (see also comments from the Fortran community). We have to use Meson as the building system for the Python binding if we want it to be accepted by SciPy.

Thanks.

@jschueller
Copy link
Collaborator Author

jschueller commented Sep 12, 2023

we dont have to move to meson, integrating into scipy will consist in copying the sources there into their own build system, and even if we moved to meson I doubt much would be reusable anyways.

@zaikunzhang
Copy link
Member

zaikunzhang commented Sep 12, 2023

to use the fortran libs directly the iso_c_binding part should be moved inside the fortran lib, but I dont think it would affect the performance nor the maintenance since the c binding would probably have to keep in sync with the fortran side anyways, as for performance its only for IO, so I dont think it matters

Sorry, I believe this matters a lot, especially to the SciPy maintainers. A mixture of Fortran and Python is already considered unpleasant. Getting another language involved will make things much more complicated --- maybe not to ourselves, but remember that other people may have to maintain our code in the (not very far) future.

Above all, it is not necessary to traverse the C interface if Python wants to communicate with iso_c_binding Fortran subroutines. So I do not see why the C interface should be invited into the play.

to use the fortran libs directly the iso_c_binding part should be moved inside the fortran lib

Is it the compiled version or the source code that has to be moved?

Thanks.

@zaikunzhang
Copy link
Member

zaikunzhang commented Sep 12, 2023

we dont have to move to meson, integrating into scipy will consist in copying the sources there into their own build system, and even if we moved to meson I doubt much would be reusable anyways.

Sorry, this is beyond my knowledge again. So we do not need to provide or contribute to the building facility that "compiles the Fortran code and makes it callable in Python", in order to get Python binding integrated into SciPy?

I thought preparing such a building system and ensuring it works on all platforms was the most challenging part.

@jschueller
Copy link
Collaborator Author

jschueller commented Sep 12, 2023

I guess all the c/bobyqa_c.f90, c/cintrf.f90, ... source files should be moved to /fortran, else the wont be compiled into libprimaf.so and in turn we wont be able to use it in the Python layer instead of libprimac.so like it is done now

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

check-spelling found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

python/examples/example_cobyla.py Fixed Show fixed Hide fixed
python/examples/example_cobyla.py Fixed Show fixed Hide fixed
python/prima.py Fixed Show fixed Hide fixed
python/prima.py Fixed Show fixed Hide fixed
python/prima.py Fixed Show fixed Hide fixed
@github-actions

This comment has been minimized.

python/prima.py Fixed Show fixed Hide fixed
python/prima.py Fixed Show fixed Hide fixed
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@zaikunzhang
Copy link
Member

zaikunzhang commented Sep 12, 2023

Comments by @andyfaff, copied from scipy/scipy#18118

I quickly glanced over the Python binding code, there's pretty much no comments on it, so it'll harder to go through.

  • New solvers being added to scipy should ideally use the optimize.NonlinearConstraint / optimize.LinearConstraint syntax for specifying constraints, and use optimize.Bounds for lower/upper bounds.
  • the build system for scipy uses meson, not cmake.
  • if possible the minimiser should provide a callback ability to update the user on the progress of the minimisation.
  • the minimiser should have the ability to add extra user defined args, f(x, *args).
  • bindings should follow existing approaches, i.e. f2py/cython/..., no SWIG or handwritten C extension modules.
  • all fortran code should be re-entrant.

@zaikunzhang
Copy link
Member

temporarily delete other tests

Please remember to get them back before we merge the PR :) Thank you.

@github-actions

This comment has been minimized.

@zaikunzhang
Copy link
Member

Hi @jschueller Julien,

This is for your reference: https://github.com/orgs/libprima/discussions/79 .

I suppose we are currently working on the basic interface defined there, which will be sufficient for the inclusion of PRIMA in SciPy.

Thanks.

python/prima.py Fixed Show fixed Hide fixed
python/prima.py Fixed Show fixed Hide fixed
@github-actions

This comment has been minimized.

@jschueller jschueller marked this pull request as ready for review September 21, 2023 18:52
@jschueller
Copy link
Collaborator Author

yes, lets keep things things basic here
I think I might be done here, what do you think ?

@zaikunzhang
Copy link
Member

Thank you @jschueller Julien for the huge efforts! This is very nice.

Things I have noted

  1. Matrices like Aineq should be 2D arrays (what are the most common representations of matrices in Python?).
  2. As mentioned by the SciPy maintainers, we should use the standard representations of constraints in SciPy.

New solvers being added to scipy should ideally use the optimize.NonlinearConstraint/optimize.LinearConstraint syntax for specifying constraints, and use optimize.Bounds for lower/upper bounds.

I am ignorant about Python, so @ragonneau Tom please review.

python/prima.py Fixed Show fixed Hide fixed
python/prima.py Fixed Show fixed Hide fixed
python/prima.py Fixed Show fixed Hide fixed
@github-actions

This comment has been minimized.

@jschueller
Copy link
Collaborator Author

jschueller commented Sep 25, 2023

  • we can now use 2d/1d numpy array for matrices Aineq, bineq etc
  • I prefer to not use scipy object forw now and keep the interface simple (basic interface)

@zaikunzhang
Copy link
Member

I prefer to not use scipy object forw now and keep the interface simple (basic interface)

So this interface is not for the integration with SciPy?

@jschueller
Copy link
Collaborator Author

its a basic layer, including in scipy will require more functions on top of that

@zaikunzhang
Copy link
Member

its a basic layer, including in scipy will require more functions on top of that

Will this binding include such functions as well?

@jschueller
Copy link
Collaborator Author

no, just the api will change

@zaikunzhang
Copy link
Member

no, just the api will change

Sorry, I did not get it. Could you elaborate a bit more? How will it change, and how will that facilitate the integration with SciPy? Thank you.

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

3 participants