Feature: extend the partition algorithm to handle weights

[dutkalex]

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 the t8_forest_set_partition function 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.
• In the general case, a single pass algorithm is not possible because the total weight of the forest wan't be known a priori and must be computed (first traversal of the forest) to then be able to define the new partition boundaries (second traversal). This means that each element weight is computed twice with the current implementation. If the weight function is expensive to evaluate, this could cause performance problems. There are basically two ways to work around this: the results could either be cached during the first evaluation (this means that we need to allocate a temporary array), or the weights could alternatively be computed and stored in an array ahead-of-time by the user, before calling the partition function. In the second case the user then simply provides a weight function that looks up the precomputed weights. This second option was preferred because it gives the user greater flexibility, and because it offers optimal performance in the case where the weight function is cheap to evaluate which is probably the default case ("Don't pay for what you don't use" principle).

All these boxes must be checked by the AUTHOR before requesting review:

• The PR is small enough to be reviewed easily. If not, consider splitting up the changes in multiple PRs.
• The title starts with one of the following prefixes: Documentation:, Bugfix:, Feature:, Improvement: or Other:.
• If the PR is related to an issue, make sure to link it.
• The author made sure that, as a reviewer, he/she would check all boxes below.

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

• The reviewer executed the new code features at least once and checked the results manually.
• The code follows the t8code coding guidelines.
• New source/header files are properly added to the CMake files.
• The code is well documented. In particular, all function declarations, structs/classes and their members have a proper doxygen documentation.
• All new algorithms and data structures are sufficiently optimal in terms of memory and runtime (If this should be merged, but there is still potential for optimization, create a new issue).

Tests

• The code is covered in an existing or new test case using Google Test.
• The code coverage of the project (reported in the CI) should not decrease. If coverage is decreased, make sure that this is reasonable and acceptable.
• Valgrind doesn't find any bugs in the new code. This script can be used to check for errors; see also this wiki article.

If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):

• Should this use case be added to the github action?
• If not, does the specific use case compile and all tests pass (check manually).

Scripts and Wiki

• If a new directory with source files is added, it must be covered by the script/find_all_source_files.scp to check the indentation of these files.
• If this PR introduces a new feature, it must be covered in an example or tutorial and a Wiki article.

License

• The author added a BSD statement to doc/ (or already has one).

[holke]

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.
Or keep the standard version as is - but i also see your point of avoiding code duplication which is very important.

[dutkalex]

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.

[dutkalex]

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). I'm not quite convinced this is really usable in practice however, especially if the forest has just been refined or coarsened prior to entering the partition algorithm. Do you have ideas on what would be the right function signature in the general case?
Since the weight function is always used on a committed forest, then all the non mutating public API functions can be used by this weight function to query for additional information if needed.

[codecov]

Codecov Report

❌ Patch coverage is 92.18750% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.46%. Comparing base (17e31ac) to head (4665876).
⚠️ Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
src/t8_forest/t8_forest_partition.cxx	92.06%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1889      +/-   ##
==========================================
+ Coverage   77.42%   77.46%   +0.04%     
==========================================
  Files         112      112              
  Lines       19037    19084      +47     
==========================================
+ Hits        14739    14784      +45     
- Misses       4298     4300       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dutkalex]

I have refactored to use t8_shmem_array_allgatherv in the weighted case as you suggested @holke, but it seems like I'm still missing something to make this work...
https://github.com/dutkalex/t8code/blob/weighted-partition/src/t8_forest/t8_forest_partition.cxx#L519

[dutkalex]

I hope you don't mind that I do not use the DeferToScopeExit: While I like the idea, it felt like an overkill to me and I instead packed the weighted partitioning in an else statement

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.

Also, I added the flag set_weighted_partitioning which I agree is not required, but in line with how t8code handles other flags and options -- although it is a somewhat ugly solution...

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 nullptr before calling the weight function.

The only really new thing I added is the test case.

Perfect 😊

[spenke91]

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 nullptr before calling the weight function.

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.

[spenke91]

@dutkalex I gathered some opinions from the other developers and got rid of the extra flag as you suggested 👍

[dutkalex]

@dutkalex I gathered some opinions from the other developers and got rid of the extra flag as you suggested 👍

Ok, perfect then 😊

[Davknapp]

Since @spenke91 already reviewed most of the code I only had a detailled look at the tests that were added by him.

Looks good in general, only requires some small changes!