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

C interface #28

Merged
merged 4 commits into from
Sep 11, 2023
Merged

C interface #28

merged 4 commits into from
Sep 11, 2023

Conversation

jschueller
Copy link
Collaborator

@jschueller jschueller commented May 29, 2023

This adds a C interface to the Fortran library thanks to the iso_c_binding module:

prima.h:

typedef void (*prima_function)(const double *x, double *f);
typedef void (*prima_function_con)(const double *x, double *f, double *con);

int prima_cobyla(prima_function_con calcfc, int n, int m, double *x, double rhobeg, double rhoend, int *maxfun, int iprint, void *state);
int prima_bobyqa(prima_function calfun, int n, double *x, double *xl, double *xu, double rhobeg, double rhoend, int *maxfun, int iprint, void *state);
int prima_newuoa(prima_function calfun, int n, double *x, double rhobeg, double rhoend, int *maxfun, int iprint, void *state);
int prima_uobyqa(prima_function calfun, int n, double *x, double rhobeg, double rhoend, int *maxfun, int iprint, void *state);

Some fortran modules have to be renamed to allow building everything into one single fortran library.

This also introduces a CMake build system to build both the Fortran library and the C library in a cross-platform manner.

Comments welcome

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.

@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.

@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.

@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.

@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.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@jschueller jschueller marked this pull request as ready for review May 31, 2023 12:00
@github-actions

This comment has been minimized.

@jschueller
Copy link
Collaborator Author

@zaikunzhang I think this is ready for review
I also want to add a data pointer & return code to the callback to allow user abort, but that would involve more modifications to the fortran side, so lets hear your thoughts on this before

@zaikunzhang
Copy link
Member

@zaikunzhang I think this is ready for review

I also want to add a data pointer & return code to the callback to allow user abort, but that would involve more modifications to the fortran side, so lets hear your thoughts on this before

Dear @jschueller ,

Thank you very much for the wonderful contribution! Sorry for the slow response.

Is there a way to avoid renaming the Fortran files/subroutines, maybe by building the solvers one by one instead of all together?

Another monitor comment is that you may consider using https://github.com/marketplace/actions/setup-fortran to get Fortran compilers in CI.

Many thanks and best regards,
Zaikun

@jschueller
Copy link
Collaborator Author

jschueller commented Jun 1, 2023

While we could split the solvers, it would be impractical and also result in separate c libraries, this is probably not what we want because it would prevent to use more than one solver at at time in the client codes.
Also it speeds up compilation because common code does not need to be recompiled each time.

I will check out setup-fortran, thanks

@github-actions

This comment has been minimized.

@jschueller
Copy link
Collaborator Author

oh, yes, there are usually no subdirs under /lib, or at least its less frequent

@zaikunzhang
Copy link
Member

  • the external example is mostly there to provides the CMakeLists needed to use prima from another cmake projects, as opposed to the other "builtin" examples that are built from the same prima cmake project

Sorry, I still do not get the difference. What if we regard all examples under c/examples/ and fortran/example/ as external?

What about this one?

@zaikunzhang
Copy link
Member

@zaikunzhang
Copy link
Member

5. Do you really intend to run the test every 5 minutes?

8. I do not think it is a good idea to print cstrv for unconstrained problems.

Any comments?

@zaikunzhang
Copy link
Member

zaikunzhang commented Sep 10, 2023

Could you direct me to the most recent run of the stress test? I have not found tests that run several hours and get cancelled by GitHub.

it should normally run 6 hours before it is canceled by GitHub. See https://github.com/orgs/community/discussions/58716#discussioncomment-6274151 for how to mark such workflows as successful.

Did you need this? Without doing so, the workflow will always be marked as a failure.

Thanks.

@jschueller
Copy link
Collaborator Author

jschueller commented Sep 11, 2023

  • there is no difference in the C code itself, but examples from c/examples/ and fortran/example/ are already part of the build system of prima (built by the root CMakeLists), so they are not fully independent of prima in that regard (unless you copy the CMakeLists of the external example or write your own makefile of course), one can argue that the external folder is mainly there to showcase how to write an CMakeLists of a new project that links against prima
  • it is there for instrumentation purpose; I wanted to see how many iterations I could allow before the 6h timeout,
    it could also be useful to tell if the code makes progress and is not frozen
  • I modified the code to run it once every week no every 5 minutes of course
  • I think we can keep the output cstrv, it will be zero in these cases, no big deal
  • cron jobs will not be run until it is part of the default branch, so until merging

@zaikunzhang
Copy link
Member

  • one can argue that the external folder is mainly there to showcase how to write an CMakeLists of a new project that links against prima

For me, every example is to showcase how to use prima, as suggested by the word "example". That's why I suggest treating all examples as "external examples". This aligns with what I mentioned earlier:

#28 (comment)
#28 (comment)

@zaikunzhang
Copy link
Member

  • it is there for instrumentation purpose; I wanted to see how many iterations I could allow before the 6h timeout,
    it could also be useful to tell if the code makes progress and is not frozen

I suppose this is not needed.

@jschueller
Copy link
Collaborator Author

jschueller commented Sep 11, 2023

  • ok, lets treat all as external
  • lets make that an optional debug option then

Rename modules to allow to compile everything into a single library
@zaikunzhang
Copy link
Member

Hi @jschueller ,

I tried the following.

git clone git@github.com:jschueller/prima.git -b cport
cd prima
cmake -S . -B . -DCMAKE_INSTALL_PREFIX=install
cmake --build . --target install
cd c/examples/cobyla
cmake -S . -B build -DCMAKE_INSTALL_PREFIX=install -DPRIMA_DIR=$PWD/../../../install/lib/cmake/prima/
cmake --build build --target install

Then, under c/examples/cobyla/install, there was program. However, when I tried

./program

I got the following:

./program: error while loading shared libraries: libprimac.so: cannot open shared object file: No such file or directory

I confirmed that libprimac.so is under /prima/install/lib.

Did I overlook something?

BTW, I understand it is easier to call the binary program, but it might be better to be called cobyla_example in this case.

Thank you.

@jschueller
Copy link
Collaborator Author

jschueller commented Sep 11, 2023

you might want to either set the environment variable LD_LIBRARY_PATH to the path of libprimac.so, or pass -DBUILD_SHARED_LIBS=OFF when configuring prima with cmake
it is also possible to set an rpath with cmake with CMAKE_INSTALL_RPATH, but it would require to set them at both levels

lets advise to build static libs from the README, that would be the easiest way

@zaikunzhang
Copy link
Member

zaikunzhang commented Sep 11, 2023

The new code in README works. Great!

  1. Could we change the name of the binary to *a_example?
  2. Should we / is it common to run the binary after it is compiled?
  3. In the Fortran case, a mod subfolder is created under fortran/example/cobyla/install. It is empty. Is it standard to keep this directory? If no, could we avoid producing it?

Thanks.

This adds a simplified C interface to the Fortran library.

This also introduces a CMake build system to build both the Fortran library and the C library.
@jschueller
Copy link
Collaborator Author

  1. ok
  2. ok
  3. that should not happen with a build folder different than the source folder, I updated the README to put everything in /build

@zaikunzhang
Copy link
Member

zaikunzhang commented Sep 11, 2023

  1. ok
  2. ok
  3. that should not happen with a build folder different than the source folder, I updated the README to put everything in /build

I really appreciate your quick action. Thank you very much.

1 is fixed on my side. Have you pushed the changes for 2 and 3? I do not see changes in the behavior.

@zaikunzhang
Copy link
Member

a mod subfolder is created under fortran/example/cobyla/install.

Sorry, this is wrong. It is under fortran/example/cobyla.

@jschueller
Copy link
Collaborator Author

yes, it should not get created anymore with the new README

@zaikunzhang zaikunzhang merged commit 0a4c979 into libprima:main Sep 11, 2023
1 check passed
@zaikunzhang
Copy link
Member

Merged! Thank you very much @jschueller !

Some links are broken in REAMDE. I will fix them.

@zaikunzhang
Copy link
Member

Hi @jschueller , I modified the README after the merge, particularly the Modern Fortran and C sections. Do they look correct to you?

In particular, are these correct, especially the location of compiled files?

prima/README.md

Lines 214 to 215 in 9e19a7d

This should create the **`primaf`** library for Fortran usage, located in the `install/lib` directory
to be used with the module files in `install/include/prima/mod`.

prima/README.md

Line 231 in 9e19a7d

which should also create the **`primac`** library for C compilation, located in `install/lib` to be used with the **`prima.h`** header in `install/include/prima`.

Thanks.

@jschueller jschueller deleted the cport branch September 11, 2023 18:33
@jschueller
Copy link
Collaborator Author

I believe so

@zaikunzhang
Copy link
Member

Thank you! @jschueller

@zaikunzhang
Copy link
Member

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

4 participants