Feature: extend the partition algorithm to handle weights #1889
Conversation
|
Thanks again for this addition! Since 95% of our users do not use weighted partitioning but the classical one and the weighted does add overhead i would argue we should at least skip the weight computation loop in standard mode. |
That's an interesting idea. Maybe we can get the best of both worlds with this kind of "fast-forward" behavior in the simple case. I will look into that. |
|
I wonder what should be the input of the weight function. Currently it takes a forest, a local tree id and an element id (within that tree). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1889 +/- ##
==========================================
+ Coverage 77.05% 77.09% +0.04%
==========================================
Files 112 112
Lines 18959 19006 +47
==========================================
+ Hits 14608 14653 +45
- Misses 4351 4353 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I have refactored to use |
Co-authored-by: spenke91 <[email protected]>
|
Hi @spenke91! |
|
Hi @dutkalex, I just updated the PR. Most of the changes we made were pretty much the same. I hope you don't mind that I do not use the |
These things are matter of a personal taste: I like to avoid nested if blocks and prefer early returns instead, but your solution is arguably just as readable and maintainable, so LGTM.
I would argue that it is redundant and also an opportunity for inconsistency (and therefore bugs and added maintenance work), and that I don't really see the value in adding this. However, at the end of the day this is your call since you guys are the ones which will have to maintain the code. If you really want to go down that road, I would strongly advise to add a check for
Perfect 😊 |
Frankly, I agree with you and the check for the nullptr is a good point! I will ask one of the more experienced t8code developers for an oppinion on this -- and then either change it or at least add the nulltptr check. |
|
@dutkalex I gathered some opinions from the other developers and got rid of the extra flag as you suggested 👍 |
Ok, perfect then 😊 |
src/t8_forest/t8_forest_general.h
Outdated
| /** Set a user-defined weight function to guide the partitioning. | ||
| * \param [in, out] forest The forest. | ||
| * \param [in] weight_callback A callback function defining element weights for the partitioning. | ||
| * \pre \a weight_callback must be free of side-effects, the behavior is undefined otherwise |
There was a problem hiding this comment.
What do you mean by side-effects?
There was a problem hiding this comment.
It should not change the forest for example
There was a problem hiding this comment.
Yes, and it should not modify some global state such that calling twice the function on the same element could give different results also.
| t8_debugf ("Create uniform base forest.\n"); | ||
|
|
||
| // Initial uniform refinement level | ||
| const int level = 2; |
There was a problem hiding this comment.
What is the current run-time of the test? Maybe we can allow a higher-level for the production-mode.
There was a problem hiding this comment.
In the github workflow, it took 3.6s for Debug and 2.2s in Release
You mean setting a higher level depending on the test level?
To be honest, I don't think it adds any value to the test case but simply increases its workfload. 🤔
We simply partition the forest's leaf elements, so the depth of the tree does not really change anything. In principle, level 1 would already be enough I think, but I set level 2 because it reduces the number of tested forests with very few elements.
There was a problem hiding this comment.
FYI: Tested locally, going to level 3 roughly doubles the runtime, level 4 brought already an increase of roughly a factor of 14.
4 participants
Describe your changes here:
This PR makes it possible to define element weights for the partitioning algorithm. This is achieved via a callback pointer. In the null case, the old unweighted algorithm is used.
This introduces one breaking change in the public API, in thet8_forest_set_partitionfunction signature. All the client code in the repo has been updated, but this will require an update of external user code:t8_forest_set_partition(forest, set_from, set_for_coarsening)=>t8_forest_set_partition(forest, set_from, set_for_coarsening, nullptr)Design tradeoffs:
The new algorithm replaces the old one, even if no weight function is provided. Keeping the old algorithm around could possibly result in slightly better performance since it requires less work, but this would come at the cost of increased maintenance and the need for duplicated testing to cover both code paths. Given that the partition algorithm is not a performance bottleneck in practice, the single code path solution was preferred.The final design includes the unweighted case as a fast "early exit" case.All these boxes must be checked by the AUTHOR before requesting review:
Documentation:,Bugfix:,Feature:,Improvement:orOther:.All these boxes must be checked by the REVIEWERS before merging the pull request:
As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.
General
Tests
If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):
Scripts and Wiki
script/find_all_source_files.scpto check the indentation of these files.License
doc/(or already has one).