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 algo enum #114

Merged
merged 2 commits into from
Dec 4, 2023
Merged

Add algo enum #114

merged 2 commits into from
Dec 4, 2023

Conversation

jschueller
Copy link
Collaborator

This simplifies even further the API with an enum to choose the algorithm instead of the 5 different functions for each algorithm:

enum prima_algorithm = {PRIMA_COBYLA, PRIMA_BOBYQA, ...}
int prima_minimize(int algorithm, prima_problem *problem, prima_options *options, prima_result *result);

@jschueller
Copy link
Collaborator Author

/cc @nbelakovski

@nbelakovski
Copy link
Contributor

Other than the comment about using the enum type (which occurs in 3 or 4 other places besides the one I mentioned) it looks good to me.

This simplifies even further the API with an enum to choose the
algorithm instead of the 5 different functions for each algorithm:
```
enum prima_algorithm = {PRIMA_COBYLA, PRIMA_BOBYQA, ...}
int prima_minimize(int algorithm, prima_problem *problem, prima_options *options, prima_result *result);
```
c/include/prima/prima.h Fixed Show fixed Hide fixed
c/include/prima/prima.h Fixed Show fixed Hide fixed
c/prima.c Fixed Show fixed Hide fixed
c/tests/data.c Outdated Show resolved Hide resolved
c/tests/stress.c Outdated Show resolved Hide resolved
@nbelakovski
Copy link
Contributor

LGTM, I'll push on @zaikunzhang to get this merged and then rebase the callback PR and my python bindings branch

@TLCFEM
Copy link

TLCFEM commented Nov 30, 2023

TBH, I personally prefer the version before this PR. The reason is, there will be some users, that do not need all five solvers (like myself). Then one can selectively compile and include some but not all source files in the downstream project.

Using a single entry point prima_minimize implies I have to compile everything even if I only use one solver.

@nbelakovski
Copy link
Contributor

nbelakovski commented Dec 1, 2023

@TLCFEM A couple questions:

  1. Why is it a problem to compile everything? I'm not saying it isn't a problem, for example I see that the final primaf library is ~1MB which might not be OK for certain embedded contexts, I'm just curious about your use cases and motivations.
  2. Which of the following proposed solutions might work best for you?
    1. Since the relevant functions are exposed to C, you could just use cobyla_c (or whatever algorithm you're interested in) directly without going through prima_minimize. You'd lose the convenience of the API with the prima_problem_t etc., but if you're only interested in one algorithm then maybe that adds a layer of indirection you don't want anyway?
    2. Maintaining the existing API alongside the one proposed in this PR
    3. Something else?

I'm also curious as to how you're solving this problem at the moment. Do you have some custom build scripts you use on top of prima to build only the algorithm you want?

@TLCFEM
Copy link

TLCFEM commented Dec 1, 2023

It may not be a problem for some, but could be a problem for others. Maybe they simply just do not want to include dead code (dead in the sense that they will not use in their particular use cases). Keeping those solvers indepedent allows linkers to omit unused code, reduce binary size, etc.

Similar logic can be often seen elsewhere. I also do not use boost for the same reason.

I would say adding a unified API while keeping the indepedent APIs would be good. This allows flexibility and for people who care about it and really do not want it, they can just remove the corresponding part.

@TLCFEM
Copy link

TLCFEM commented Dec 1, 2023

Currently I am not using it in production. I see the implemention is mostly sequential. I presume it can be easily parallelised by either omp or third-party lapack implementation as I'd like to discuss with you here. I do plan to employ some solvers in large scale (thousands to millions of DoFs) problems. But atm, I am mostly trying small problems, and checking how the performance would be with parallelised linear algebra operations.

@nbelakovski
Copy link
Contributor

You make a good point about boost, but this library won't grow beyond these 5 solvers (in fact it doesn't and won't include COBYQA, an improvement over COBYLA, because the focus of this library is on Powell's solvers).

Also, unlike boost, we have very few external dependencies (in many cases none).

I could see an argument for structuring the code in such a way as to put all of the common code into a common library, and then make separate libraries on top of it for the various algorithms, and then a "master" library on top of all those to just provide all of them, but I think that in the interest of avoiding premature optimization I would prefer to defer that work until such a time as it becomes a problem.

For what it's worth, in the fortran examples, we have makefiles that compile only the necessary algorithm, so at least there is a reference for compiling jsut the algorithm you need.

@TLCFEM
Copy link

TLCFEM commented Dec 1, 2023

You make a good point about boost, but this library won't grow beyond these 5 solvers (in fact it doesn't and won't include COBYQA, an improvement over COBYLA, because the focus of this library is on Powell's solvers).

Also, unlike boost, we have very few external dependencies (in many cases none).

I could see an argument for structuring the code in such a way as to put all of the common code into a common library, and then make separate libraries on top of it for the various algorithms, and then a "master" library on top of all those to just provide all of them, but I think that in the interest of avoiding premature optimization I would prefer to defer that work until such a time as it becomes a problem.

For what it's worth, in the fortran examples, we have makefiles that compile only the necessary algorithm, so at least there is a reference for compiling jsut the algorithm you need.

Yeah, I am not very concerned as I can just extract part of the library and modify the c source files accordingly. And one can always directly call the _c fortran version. Don't get me wrong, a unified API is for sure good. What I'd like to say is that the indepedent APIs offer extra flexibility so maybe they can coexist with the unified one.

@zaikunzhang zaikunzhang merged commit 2239699 into libprima:main Dec 4, 2023
5 checks passed
@jschueller jschueller deleted the algo branch December 4, 2023 09:55
@nbelakovski
Copy link
Contributor

Circling back to this, @zaikunzhang and I had a discussion about @TLCFEM 's concerns before merging, @zaikunzhang was going to add a comment to this but I guess it slipped through. The main reason for not exposing the individual algorithms is that it makes it very tedious to do common things like scaling the input. pdfo, the library for exposing the F77 versions of these algorithms to Python, exposes the individual algorithms as well as a unified one, but according to @zaikunzhang experience has proved this to be a mistake.

@zaikunzhang
Copy link
Member

We should only provide one single function (e.g. prima) to the users. Exposing both prima and individual solvers will substantially complicate the logic of the code in many ways. This is a lesson we learned from PDFO.

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.

4 participants