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

metis wrapper broken with metis 5.1.1 and 5.2.1 #133

Closed
traversaro opened this issue Aug 29, 2023 · 3 comments · Fixed by #134
Closed

metis wrapper broken with metis 5.1.1 and 5.2.1 #133

traversaro opened this issue Aug 29, 2023 · 3 comments · Fixed by #134
Assignees
Labels

Comments

@traversaro
Copy link

In https://github.com/ralna/spral/blob/v2023.08.02/src/metis5_wrapper.F90#L77 the values of the elements of the moption_set enum are set: KarypisLab/METIS#71 (comment) .

However, this values have been changed between METIS 5.1.0 and 5.1.1 , see KarypisLab/METIS#71 (comment) .

I do not think this is a big problem as the official instructions and most users are using METIS 5.1.0, however I want report the problem somewhere to have a reference for people that may encounter problems with METIS >= 5.1.1 .

@jfowkes
Copy link
Contributor

jfowkes commented Aug 31, 2023

Thanks for reporting @traversaro we are aware of this issue but it’s good to have an actual GitHub issue open as this may become a bigger problem going forward.

The main issue here is that the enums from the METIS 5 header file are hard-coded in the SPRAL METIS 5 Fortran wrapper, and they change between different versions of METIS 5 (I’m not even sure if they are completely correct for version 5.1.0 atm).

Fundamentally this is a flawed design of the METIS 5 Fortran wrapper and the only way that I can see to get around this is to have a C wrapper that imports the METIS 5 C header to query the enums, essentially we can repurpose the COIN wrapper from here:
https://github.com/coin-or-tools/ThirdParty-HSL/blob/stable/2.2/metis_adapter.c
with an ISO C wrapper that exposes this on the Fortran side. We have done this successfully for COIN-HSL but it is not clear that the translation of METIS 4 options to METIS 5 options is entirely correct and this will need careful looking into. However this clearly needs to be done.

@jfowkes jfowkes added the bug label Aug 31, 2023
@jfowkes jfowkes self-assigned this Aug 31, 2023
@jfowkes jfowkes added this to the Second New Release milestone Aug 31, 2023
@jfowkes
Copy link
Contributor

jfowkes commented Sep 4, 2023

Looking deeper into this, the only METIS 5 parameter that the SPRAL METIS 5 Fortran wrapper actually sets is METIS_OPTION_NUMBERING to enable Fortan-style (1-based) numbering in the returned permutations perm,iperm.

So a quick fix would be to simply use the default C-style (0-based) numbering and just add one to the returned perm,iperm integer arrays. The question is, would this introduce significant additional overhead in the wrapper?

@jfowkes jfowkes linked a pull request Sep 6, 2023 that will close this issue
@traversaro
Copy link
Author

Thanks a lot @jfowkes !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants