Feature new for hybrid #768

Davknapp · 2023-09-25T14:21:02Z

PR to enable the hybrid new.

Current TODOs:

Enable all tests
understand why t8_cmesh_hybrid_new.cxx fails for 'mpirun -np 7 ./example/cmesh/t8_cmesh_hybrid_new -l4 -f6 -p -m1' and fix it.

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 Makefiles
The code is well documented
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

Github action

The code compiles without warning in debugging and release mode, with and without MPI (this should be executed automatically in a github action)
All tests pass (in various configurations, this should be executed automatically in a github action)

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/tutorial and a Wiki article.

Licence

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

Co-authored-by: holke [email protected]

…efines irregularly

Davknapp · 2023-10-23T13:35:42Z

Merging with main solved the second issue

Davknapp · 2023-10-23T14:57:47Z

After Debugging-session with @lukasdreyer
Current Problem
Vertex mesh with a single vertex on level 11 with 2 procs.
proc 0 computes first_child 0 and last_child -1 and sets its first tree to -1, which should not be possible.

src/t8_cmesh/t8_cmesh_cxx.cxx

Removed debugging output Enhanced scope of variables More const params

Co-authored-by: Johannes Holke <[email protected]>

example/cmesh/t8_cmesh_hybrid_new.cxx

holke

very good contribution - obviously with such a big PR i have some comments, but i see no major problems.

You should add addtional test cases to catch the boundary cases.
For example testing when the first tree is not on the first process.

src/t8_cmesh/t8_cmesh_cxx.cxx

holke · 2023-12-08T10:20:52Z

src/t8_cmesh/t8_cmesh_cxx.cxx

+{
+  return t8_A_times_B_over_C_intA (process, global_num_elements, mpisize);
+}
+
 void
 t8_cmesh_uniform_bounds (t8_cmesh_t cmesh, int level, t8_scheme_cxx_t *ts, t8_gloidx_t *first_local_tree,


we can now rename this function to
t8_cmesh_uniform_bounds_element_counts_are_equal (or similar name)
and call it each time that for all eclasses the "number of elements at level l" is the same.
It must not even be 2^n or k^n, just the same on the given level.

holke · 2023-12-08T10:24:05Z

src/t8_cmesh/t8_cmesh_cxx.cxx

+}
+
+/* TODO: Shared trees, binary search in offset-array to avoid recv_any,
+ * use partition_given to partition the cmesh*/


is this TODO still to do?

holke · 2023-12-08T10:24:36Z

src/t8_cmesh/t8_cmesh_cxx.cxx

+#endif
+
+  /* TODO: Clean up size_t and gloidx_t data types, ensure that each variables has the 
+   *          matching type. */


is this TODO still to do?

holke · 2024-01-10T14:05:24Z

src/t8_cmesh.h

+ * \param [in] comm         The communicator 
+ */
+void
+t8_cmesh_uniform_bounds_hybrid (const t8_cmesh_t cmesh, const int level, const t8_scheme_cxx_t *scheme,


we agreed to change the name, since the old version can also be used for hybrid schemes.

holke · 2024-01-11T10:42:30Z

src/t8_cmesh/t8_cmesh_cxx.cxx

+      = query_data->num_procs - 1
+        - t8_A_times_B_over_C_intA (query_data->num_procs, mirror_element_index, query_data->global_num_elements);
+
+    //       (int)(((long double)mirror_element_index/query_data->global_num_elements)*query_data->num_procs);


can we delete this comment?

holke · 2024-01-11T10:42:58Z

src/t8_cmesh/t8_cmesh_cxx.cxx

+  const t8_gloidx_t mirror_element_index = query_data->global_num_elements - element_index - 1;
+  int first_proc_rank;
+
+  t8_debugf ("[H] tree %li el %li mirror %li\n", pure_local_tree, element_index, mirror_element_index);


is this print still needed?

holke · 2024-01-11T10:43:14Z

src/t8_cmesh/t8_cmesh_cxx.cxx

+     * add the first process. */
+    first_proc_adjusted = first_proc_rank - query_data->process_offset;
+  }
+  t8_debugf ("[H] ptree %zd, first element %li, on proc %zd\n", pure_local_tree, element_index, first_proc_adjusted);


is this print still needed?

src/t8_cmesh/t8_cmesh_cxx.cxx

holke · 2024-01-11T10:58:25Z

src/t8_cmesh/t8_cmesh_cxx.cxx

+t8_cmesh_uniform_bounds_hybrid (const t8_cmesh_t cmesh, const int level, const t8_scheme_cxx_t *scheme,
+                                t8_gloidx_t *first_local_tree, t8_gloidx_t *child_in_tree_begin,
+                                t8_gloidx_t *last_local_tree, t8_gloidx_t *child_in_tree_end, int8_t *first_tree_shared,
+                                sc_MPI_Comm comm)


This function is 600 lines long - uff.
Consider outsourcing some code into subroutines.
For example

partitioned and non-partitioned computation

sending of start message

sending of end message

receiving of start message

receiving of end message

Co-authored-by: Johannes Holke <[email protected]>

Knapp and others added 16 commits September 23, 2022 09:22

Add cmesh uniform bounds for hybrid meshes.

b8e3dd4

Co-authored-by: holke [email protected]

Add all missing functions to make code compilable

553747c

Add example

4616091

move is_irregular function and use hybrid_bounds only when a forest r…

a853e40

…efines irregularly

Some optimization

6021fe4

use t8_cmesh_uniform_bounds_hybrid in cmesh_partition

c4d4b3b

Update variables according to their scope and made them const

f271d06

Merge branch 'main' into feature-New_for_hybrid

9e103cc

Update to current main and indentation

d15062e

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

f026cde

Merge Clean-up

c6d505e

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

b95c49a

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

104da7c

Add missing \ to Makefile

7c90788

Update headers used in example

ce05a7d

Use hybrid_bounds algorithms

4add0d6

Davknapp added New feature Adds a new feature to the code draft Enhance the visibility that this is a draft. labels Sep 25, 2023

lukasdreyer mentioned this pull request Oct 19, 2023

Fix bounds for partitioning the cmesh according to uniform forest #798

Open

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

18ebd66

WIP: Debugging cmesh_uniform_bounds_hybrid

1ef18ed

lukasdreyer reviewed Oct 24, 2023

View reviewed changes

src/t8_cmesh/t8_cmesh_cxx.cxx Outdated Show resolved Hide resolved

lukasdreyer and others added 6 commits October 25, 2023 16:00

Fix special cases for empty procs

58892c8

Reintroduce cmeshes with pyramids

8ce611e

Fix number of vertices for pyramid cmesh

cbbb980

Fix compiler warnings

4c26c2f

Fix more compiler errors

255fea7

Clean-up

a8314dc

Removed debugging output Enhanced scope of variables More const params

Davknapp and others added 5 commits November 13, 2023 11:04

Apply suggestions from code review

2ce5b2c

Co-authored-by: Johannes Holke <[email protected]>

WIP: Minimize example

49fb50c

Added description to t8_cmesh_uniform_bounds_hybrid

b2f81b9

More const

800b716

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

bf2ca8d

holke reviewed Nov 24, 2023

View reviewed changes

example/cmesh/t8_cmesh_hybrid_new.cxx Outdated Show resolved Hide resolved

holke requested changes Jan 11, 2024

View reviewed changes

holke assigned Davknapp and unassigned holke Jan 11, 2024

Davknapp and others added 21 commits January 26, 2024 14:34

Merge

a49cbc8

Rename function, add assertion

408954d

refactor function

d0539e6

Apply suggestions from code review

69aae13

Co-authored-by: Johannes Holke <[email protected]>

Indentation

229a1dc

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

f7abc12

Resolve merge errors

cf728e8

Typo fix

b92817d

Rename t8_cmesh_uniform_bounds_hybrid

fa967f6

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

294f50c

Remove comment

f8b39d5

Update to main

ce649eb

Clean-up

ef97012

Simplification of determining send_first

24ca4e9

break off uniform_bound_from_partition

e78a262

Seperate sending start message

1cddf65

update cmesh_bounds_send_start

6ab9dfa

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

1a6df19

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

14af446

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

27da81d

Merge remote-tracking branch 'origin/main' into feature-New_for_hybrid

621969d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature new for hybrid #768

Feature new for hybrid #768

Davknapp commented Sep 25, 2023 •

edited

Loading

Davknapp commented Oct 23, 2023

Davknapp commented Oct 23, 2023

holke left a comment

holke Dec 8, 2023

holke Dec 8, 2023

holke Dec 8, 2023

holke Jan 10, 2024

holke Jan 11, 2024

holke Jan 11, 2024

holke Jan 11, 2024

holke Jan 11, 2024

Feature new for hybrid #768

Are you sure you want to change the base?

Feature new for hybrid #768

Conversation

Davknapp commented Sep 25, 2023 • edited Loading

General

Tests

Github action

Scripts and Wiki

Licence

Davknapp commented Oct 23, 2023

Davknapp commented Oct 23, 2023

holke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davknapp commented Sep 25, 2023 •

edited

Loading