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

Allowing Edge Weights for the initial workload partitioning #588

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

ThrudPrimrose
Copy link

Hi, it is the essentially the same pull request but I have added the documentation for the edge and vertex weights. In a foolish mistake I have closed the last pull request, so I am opening a new one.

The text from before:
"
Adding edge weights improve the performance in most cases.

To allow this the old LtsWeights class is changed to be a class that contains NodeWeightModel and EdgeWeightModel classes as members, which respectively compute the vertex and edge cost models, depending on the given input for the strategy.

The old LtsWeights parameter that would be defined in .par file is not allowed anymore, user has to define NodeWeightModelTypeId and EdgeWeightModelTypeId.

This change also needs my PR request on PUML2, as we need to get the dual graph representation of the mesh to be able to compute edges. And we propagate the edge weights to metis, and the function to partition the graph will need the accept edgeweights as an additional parameter.
"

Now with documentation to show what it changes! Parameters are updated as well.

If this PR is merged, we also need to link to Ravil's multi constraint branch, my PR is still open there. The link:
TUM-I5/PUML2#5

ThrudPrimrose and others added 5 commits May 21, 2022 03:22
…w partitioning in respect to communicaiton cost. The user now has to set NodeWeightModelTypeId and EdgeWeightModelTypeId instead of the LtsWeights arguments that was aviable before, which is removed
Copy link
Contributor

@Thomas-Ulrich Thomas-Ulrich left a comment

Choose a reason for hiding this comment

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

Thx for the documentation. Sounds great!
Here are already some comments about the documentation.

Documentation/memory-requirements.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/parameters.par Outdated Show resolved Hide resolved
@ThrudPrimrose
Copy link
Author

I have updated everything according to the comments

Copy link
Contributor

@Thomas-Ulrich Thomas-Ulrich left a comment

Choose a reason for hiding this comment

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

Thx for the changes, everything is already much clearer.
Here is a new round of comments on the documentation.

Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
Documentation/workload-partitioning.rst Show resolved Hide resolved
Documentation/workload-partitioning.rst Outdated Show resolved Hide resolved
src/Reader/readpar.f90 Show resolved Hide resolved
src/Reader/readpar.f90 Outdated Show resolved Hide resolved
src/Reader/readpar.f90 Outdated Show resolved Hide resolved
src/Numerical_aux/typesdef.f90 Outdated Show resolved Hide resolved
src/Numerical_aux/typesdef.f90 Outdated Show resolved Hide resolved
src/Initializer/time_stepping/LtsWeights/WeightsModels.cpp Outdated Show resolved Hide resolved
src/Initializer/time_stepping/LtsWeights/WeightsModels.cpp Outdated Show resolved Hide resolved
src/Initializer/time_stepping/LtsWeights/WeightsModels.cpp Outdated Show resolved Hide resolved
src/Initializer/time_stepping/LtsWeights/WeightsModels.cpp Outdated Show resolved Hide resolved
src/Initializer/time_stepping/LtsWeights/WeightsModels.cpp Outdated Show resolved Hide resolved
@ThrudPrimrose
Copy link
Author

I have tried to apply every comment you have mentioned, I will add the comment for the functions now and make another commit. Other than that I have used the format file, so it should be nice to look at. Regarding the parameters I need to change the documentation a little because in psuedocode we combine with Approximate Communication but actualyl due to the implementation of the constraints we need different EdgeWeightModelTypeIds that implement Communicaiton Approximation + plus a little bit extra, that should be in the documentation right now

@Thomas-Ulrich
Copy link
Contributor

I've tested on several setup, and it run through, (at least) without performance losses

@ThrudPrimrose
Copy link
Author

I am glad to hear that, right now I have some other urgent tasks to attend to, I will continue with the proposed fixes for the PR as soon as possible

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.

None yet

2 participants