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

Allow more functionality through system METIS #1117

Merged
merged 3 commits into from
Mar 26, 2022
Merged

Allow more functionality through system METIS #1117

merged 3 commits into from
Mar 26, 2022

Conversation

acxz
Copy link
Contributor

@acxz acxz commented Feb 21, 2022

This is a follow up PR to #570 as when it was merged I was not available to provide my review.

There is no reason to specify the full local metis.h include path, with how metis is treated in cmake, metis.h works for both local and system metis.

See: https://github.com/borglab/gtsam/pull/570/files#diff-0106693c254359c8aceceb57d5b6c0643760cc8cce6760ccfd2d31bc6d0be2dbR26
where only metis.h is used without the ifdef.

More importantly this PR enables metis's internal api ("metislib.h") via extern C for SYSTEM_METIS. We already use the internal API for our local metis install and since we carry the header files of metis with us I do not see a problem in enabling extern "C" for system metis as well. The only problem that can occur with this is a desync between the local headers we carry for metis and what metis is compiled under at the system level. Considering metis is a very old and stable package that hasn't seen updates for a long time, I do not see this issue occurring and if it does should be easily fixable, by just updating our local metis copy.

Tagging @jlblancoc and @varunagrawal for reviews. Also @berndpfrommer ig

@acxz acxz changed the title only metis.h is needed to include both system and local metis.h file Allow more functionality through system METIS Feb 22, 2022
@dellaert
Copy link
Member

What specific issue are we fixing? Was there a problem that prompted this PR? Is there an issue associated with the problem?

@acxz
Copy link
Contributor Author

acxz commented Feb 22, 2022

What specific issue are we fixing?

Unnecessary ifdef when including <metis.h>
FindSeparator tests not run under system metis

Was there a problem that prompted this PR?

From my review comment in the last METIS PR:

Even if that is changed, system metis does not provide metislib.h which means we won't be able to use the functionality of it. Thus rendering the SYSTEM_METIS flag broken. If its going to be broken when merged in, we should just not merge it in.

Currently the ifdef guard for SYSTEM_METIS just disables the associated test. We should fix that by allowing tests to run under system metis.

Is there an issue associated with the problem?

Here is the issue: #292 (comment)
This PR implements @jlblancoc 's approach in #292 (comment)

@dellaert dellaert requested a review from jlblancoc February 22, 2022 20:30
@dellaert
Copy link
Member

Awesome. Requesting José as a reviewer, who can merge after approving.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nits but LGTM

cmake/HandleMetis.cmake Outdated Show resolved Hide resolved
cmake/HandleMetis.cmake Outdated Show resolved Hide resolved
Co-authored-by: Varun Agrawal <varagrawal@gmail.com>
@varunagrawal varunagrawal merged commit a9a4075 into borglab:develop Mar 26, 2022
@acxz acxz deleted the metis-include branch March 26, 2022 07:53
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.

3 participants