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

Avoid hardcoding METIS 5 options #134

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

jfowkes
Copy link
Contributor

@jfowkes jfowkes commented Sep 6, 2023

Resolves #133 in a fully backward compatible way by dropping the METIS_OPTION_NUMBERING=1 option (Fortran-style 1-based indexing) and instead using the METIS 5 default (C-style 0-based indexing) and adding 1 to the perm and invp arrays returned by METIS 5 (the fill-reducing permutation and inverse permutation respectively) as well as subtracting 1 from the ptr and row input arrays (matrix sparsity pattern in CSR format).

Note that there is virtually no performance loss with this change as the perm and invp arrays returned by METIS 5 are already copied in the wrapper (this is so that their return types are correct). The same is true of the input arrays ptr and row that encode the matrix sparsity pattern in CSR format which are copied so that the matrix is stored fully (not merely its lower-triangular part) as is expected by METIS 5.

@jfowkes jfowkes added the bug label Sep 6, 2023
@jfowkes jfowkes added this to the Second Major Release milestone Sep 6, 2023
@jfowkes jfowkes self-assigned this Sep 6, 2023
@jfowkes jfowkes linked an issue Sep 6, 2023 that may be closed by this pull request
@jfowkes jfowkes marked this pull request as draft September 6, 2023 14:36
@jfowkes jfowkes force-pushed the 133-metis-wrapper-broken-with-metis-511-and-521 branch 2 times, most recently from 5659c7f to b95f7c7 Compare September 6, 2023 15:22
@jfowkes
Copy link
Contributor Author

jfowkes commented Sep 6, 2023

@mjacobse I'd be interested in your thoughts on this proposed fix, does it make sense to you to do it this way?

@mjacobse
Copy link
Collaborator

mjacobse commented Sep 6, 2023

Makes sense to me 👍

One thing I would like to suggest is to not do the initial conversion from Fortran to C indexing in the implementations of the half_to_full_drop_diag interface, but instead after the calls to it in metis_order32/64. That would reduce the places in the code where this is done from 4 to 2. It would also make half_to_full_drop_diag do only the one thing that the name suggests, and not an additional somewhat unexpected index shift. And it would bring the forward and backward index-shift into the same function, right next to the Metis call for which it is actually done. Both places should even fit on screen at the same time, so it would be much easier to directly see and reason about while reading the code.

I'm not quite sure what the change from dimension(*) to dimension(:) is for in half_to_full_drop_diag? Could this also be dropped if moving the index shift outside of it?

Alternatively, if the index shift is to remain within half_to_full_drop_diag, then I think the documentation and perhaps also names should be updated accordingly. It could then also be considered to do the index shift directly while building the row2 and ptr2 arrays instead of afterwards.

@jfowkes
Copy link
Contributor Author

jfowkes commented Sep 7, 2023

Thank you, yes excellent point agreed, I will move the initial conversion over to metis_order32/64 instead of doing it in half_to_full_drop_diag which is messy.

The change from dimension(*) to dimension(:) in half_to_full_drop_diag was only there so that I could subtract one from the arrays without specifying extents so yes this can be dropped for consistency with the METIS 4 wrapper.

@jfowkes jfowkes force-pushed the 133-metis-wrapper-broken-with-metis-511-and-521 branch from 5716188 to 62909b0 Compare September 7, 2023 09:25
@jfowkes jfowkes marked this pull request as ready for review September 7, 2023 09:26
@jfowkes jfowkes merged commit 096a8e4 into master Sep 7, 2023
4 checks passed
@jfowkes jfowkes deleted the 133-metis-wrapper-broken-with-metis-511-and-521 branch September 7, 2023 13:17
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 this pull request may close these issues.

metis wrapper broken with metis 5.1.1 and 5.2.1
2 participants