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

Pulp & Patoh Integration #230

Merged
merged 18 commits into from
Nov 6, 2022
Merged

Pulp & Patoh Integration #230

merged 18 commits into from
Nov 6, 2022

Conversation

ardasener
Copy link
Contributor

@ardasener ardasener commented Nov 3, 2022

This PR includes the following contributions:

  • PulpPartition class wrapping the partitioner available here: https://github.com/HPCGraphAnalysis/PuLP
  • PatohPartition class wrapping the partitioner available here: https://faculty.cc.gatech.edu/~umit/software.html
  • Tests for the aforementioned classes and the previously introduced MetisPartition.
  • Update to the optional dependencies page in docs with a warning regarding static libraries.
  • Standardization of @param style doc comments
  • Update to the Testing action on GitHub to test with PATOH and PULP as well.
  • Default template types are replaced with their fixed-width counterparts (https://en.cppreference.com/w/cpp/types/integer).
    • By default, 32 and 64-bit signed and unsigned integers are supported for IDType and NNZType

As of now, the following work is still needed for the partitioning side of this library to be complete:

  • A class to apply partitionings to formats (Not straightforward due to the many different ways one can apply a partitioning).
  • A PartitionBase class similar to ReorderBase.
  • Passing weights to the partitioners is currently not supported. This should be added soon.
  • Some partitioners are only designed for graphs (square matrices). I believe all the hypergraph partitioners should work with matrices in general but I am not 100% sure. Regardless this should be indicated to the user perhaps by renaming the classes to XXXGraphPartitioner. And assertions can be added as well.

@ardasener ardasener self-assigned this Nov 3, 2022
@ardasener ardasener added this to the Milestone 3 milestone Nov 3, 2022
@ardasener ardasener added the type: feature Brand new functionality, features, workflows, endpoints, etc label Nov 3, 2022
@AmroAlJundi AmroAlJundi self-requested a review November 3, 2022 14:05
@Atahanak Atahanak requested review from Atahanak and removed request for AmroAlJundi November 3, 2022 14:09
graph.m = m;
graph.out_array = csr->get_col();
graph.out_degree_list = csr->get_row_ptr();
graph.vertex_weights = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should consider supporting weights in formats

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not trivial, they all support different types which is why I avoided doing so for now.

IDType *PatohPartition<IDType, NNZType, ValueType>::PartitionCSR(
std::vector<format::Format *> formats, PreprocessParams *params){

if(typeid(IDType) != typeid(int) || typeid(NNZType) != typeid(int)){
Copy link
Collaborator

Choose a reason for hiding this comment

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

constexpr like above?

Copy link
Collaborator

Choose a reason for hiding this comment

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

same here converter can be used.

Copy link
Collaborator

@Atahanak Atahanak left a comment

Choose a reason for hiding this comment

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

left minor comments.
Seems good.

@Atahanak Atahanak self-requested a review November 3, 2022 20:07
int np = pparams->num_partitions;
IDType* partition = new IDType[n];

if constexpr (std::is_signed_v<IDType> && std::is_signed_v<NNZType> &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

try converting first and if that fails raise an exception could be better.

Copy link
Collaborator

@Atahanak Atahanak left a comment

Choose a reason for hiding this comment

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

minor changes needed.

@AmroAlJundi
Copy link
Contributor

Btw, don't forget to add the += USE_PULP line to docs/conf.py and to add pages for the new partitionings to the available.md page.

Atahanak
Atahanak previously approved these changes Nov 6, 2022
…ration

# Conflicts:
#	.github/workflows/testing.yml
#	CMakeLists.txt
#	src/sparsebase/config.h.in
#	src/sparsebase/preprocess/preprocess.cc
@ardasener ardasener merged commit a688359 into develop Nov 6, 2022
@ardasener ardasener deleted the feature/pulp_integration branch November 6, 2022 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: soon High priority state: review needed type: feature Brand new functionality, features, workflows, endpoints, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants