search

ralna / spral

Sparse Parallel Robust Algorithms Library
https://ralna.github.io/spral/
Other
104 stars 27 forks source link

metis wrapper broken with metis 5.1.1 and 5.2.1 #133

Closed traversaro closed 1 year ago

traversaro commented 1 year ago

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: https://github.com/KarypisLab/METIS/issues/71#issuecomment-1697008951 .

However, this values have been changed between METIS 5.1.0 and 5.1.1 , see https://github.com/KarypisLab/METIS/issues/71#issuecomment-1697008951 .

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 commented 1 year ago

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 commented 1 year ago

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?

traversaro commented 1 year ago

Thanks a lot @jfowkes !