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

For load balancing, should cells have a "base weight" or not? #13510

Closed
bangerth opened this issue Mar 8, 2022 · 5 comments · Fixed by #13451
Closed

For load balancing, should cells have a "base weight" or not? #13510

bangerth opened this issue Mar 8, 2022 · 5 comments · Fixed by #13451
Milestone

Comments

@bangerth
Copy link
Member

bangerth commented Mar 8, 2022

#13451 is really about a broader question: The current implementation of p::d::Triangulation adds a "base weight" of 1000 to the user-provided weights for each cell. This base weight is supposed to account for the work done in the library itself to set up cell data structures, loops over all cells, etc., which we assumed to be a roughly constant cost per cell. The 1000 was chosen so that it is possible to weigh user operations in both multiples (up to ~2M times the base weight) as well as fractions (e.g., "this operation costs 0.5 times the base weight), and express this in terms of the signed int weights that are used to refer to cell weights.

#13451 proposes to get rid of the base weight. @blaisb and @peterrum argue against it, @marcfehling argues in favor of it. I'm pretty neutral. How do others feel about this as a general question? Of course, removing the base weight is an incompatible change, so that's worth keeping in mind.

@bangerth bangerth added this to the Release 10.0 milestone Mar 8, 2022
@blaisb
Copy link
Member

blaisb commented Mar 8, 2022

Just to be clear, I'm not agaisnt it. The reasons that @marcfehling suggested were good ones. I agree with the idea behind the PR and I think the reasoning is sound.

I just feel that this will break a lot of user code all of a sudden (e.g. the results will change for every code that uses some sort of load balancing that depends on something going on in certain cells). It is not a trivial change, it is more major than we think. It changes the behaviour in Lethe, but also in ASPECT (most likely), in other projects of @peterrum and these are just a few that I am aware of. I would just like if we could introduce this feature more smoothly.

At the end of the day, I'm very open to the idea. I just want to be careful about things that change the default behaviour of something that has been taken for granted for years and years.

@bangerth
Copy link
Member Author

bangerth commented Mar 8, 2022

@gassmoeller ?

@peterrum
Copy link
Member

peterrum commented Mar 8, 2022

#13451 proposes to get rid of the base weight. @blaisb and @peterrum argue against it, @marcfehling argues in favor of it. I'm pretty neutral. How do others feel about this as a general question? Of course, removing the base weight is an incompatible change, so that's worth keeping in mind.

@bangerth I am not sure what gives you the impression that I am against removing the base weight - it should be consistent in the library and the value should be reasonable.

However, I would like to point out that how is it right now it is a very hard incompatibility change. We had a large number of incompatibility changes in the last weeks: but at least they are noticeable during compile time: signature of function has changed, a function has been deprecated, or a behavior has been changed requiring the user for additional action (but there is at least a warning that tells the user what to do). Here this is not the case: the code will compile and work and will "just" result in different partitions. However, if someone manually assigns weights, he/she probably has done this intentionally (maybe after some tuning processes) so that changed partitioning is not wanted. I would like to point out also that most people do not test the actual partitioning in CI, since that would be just too unrobust and would too much depend on implementation details (about which users should not care) in deal.II/p4est/Metis and would only notice the change during some performance tests (after months after the actual change).

All what I am saying, we should do our best to make the transition as smooth as possible. With high probability the persons in this thread are not the only ones using the weighting functionalities.

@jppelteret
Copy link
Member

jppelteret commented Mar 8, 2022

One alternative, which might be a compromise between the two viewpoints, would be to expose the base weight as a static variable somewhere. At least then the user can reference the value in their own code and compose their own weighting functions without necessarily having to know what the value is. So in step-68 we could have

const unsigned int base_weight = dealii::base_cell_weight;
const unsigned int particle_weight = 10*dealii::base_cell_weight;

@bangerth
Copy link
Member Author

bangerth commented Mar 11, 2022 via email

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 a pull request may close this issue.

4 participants