search

inducer / pymetis

A Python wrapper around Metis, a graph partitioning package
http://mathema.tician.de/software/pymetis
Other
164 stars 32 forks source link

Wrapper for METIS_PartMeshNodal as part_mesh #44

Closed stoiver closed 2 years ago

stoiver commented 2 years ago

I have added a wrapper for METIS_PartMeshNodal as part_mesh.

Added in a unit test which is similar to test_tet_mesh, which tests part_graph. Indeed I changed the name of that test to test_tet_graph and added test_tet_mesh to test part_mesh.

inducer commented 2 years ago

Thanks! Looks good to me aside from the linter failure. Could you take a look?

inducer commented 2 years ago

Hmm, odd coinidence. Take a look at #43, which seems to pursue goals relatively similar goals to this PR (IIUC that is). Could the two of you come up with a joint proposal so that we can pursue that? Thanks in advance!

stoiver commented 2 years ago

@kevindugan @inducer, yes odd coincidence. It looks like #43 has wrapped METIS_PartMeshNodal as part_mesh in essentially the same way as I did in this PR (which is very similar to your wrap_part_graph).

It does seem that @kevindugan has created a more comprehensive set of tests.

Happy to go with Kevin's PR. Maybe he could pull over my test_tri_mesh from test_metis.py, though that uses meshpy.

kevindugan commented 2 years ago

@stoiver, I can certainly bring in your tet mesh test. I had considered building up a tet mesh in a similar manner to my manual quad mesh. However, since the graph partitioning tests also rely on pymesh I don't see an issue using this dependency.

@inducer, I'll also run the linter so that it conforms to your standards.

Was happy to help out on this issue. I had gone down a decent rabbit hole learning about graph partitioning methods and wanted to work out some mesh partitioning strategies which led me to the posted issue.

inducer commented 2 years ago

@stoiver, if we're going with #43, is this good to close then? AFAICT, @kevindugan has moved over the tests.

stoiver commented 2 years ago

@inducer and @kevindugan. Will close this PR as it is covered by PR #43