ForestClaw / forestclaw

Quadtree/octree adaptive PDE solver based based on p4est.
http://www.forestclaw.org
BSD 2-Clause "Simplified" License
57 stars 21 forks source link

More dimension independent forestclaw #273

Closed scottaiton closed 9 months ago

scottaiton commented 10 months ago

This is my branch where I have an octree example working.

In this branch fclaw_global_t, fclaw_domain_t, and fclaw_patch_t are dimension independent. Clawpatch likewise has been made fully dimension independent.

Basically this branch limits the code translation to files where forest claw is interacting with the underlying p4est structures, forestclaw2d.c and forestclaw3d.c. forestclaw.c accepts the dimension independent structures and redirects them to the appropriate dimension specific structures.

Any file that doesn't have a need to be dimension specific has also been renamed (e.g. fclaw2d_run.c -> fclaw_run.c).

I have tried to keep the changes to the structures minimal. I went with the style of adding a dimension specifier and two pointers to dimension specific structs:

struct {
    int dim;
    struct* d2;
    struct* d3;
}

For example, in fclaw_patch_t, xlower,ylower, etc have been moved into dimension specific structures. As long as the pointers set to NULL when not in use, this will cause the program to segfault on bad data access. This has the downside of having to allocate and free another structure for each structure, but for these types, the creation and deletion is not something the user does directly and is done in limited spot.

This is a draft pull request for now. I would like like questions, comments, and requests for changes.

donnaaboise commented 10 months ago

There are some files/routines that are specifically 2d or 3d. For example, fclaw2d_map.c. Do you keep the the fclaw2d for these files?

scottaiton commented 10 months ago

For the naming scheme, if the struct is specifically 2d/3d I kept the fclaw2d_ prefix on those classes (maps, metric data). If it's a 2d/3d specific function on a dimension independent type I went with _2d suffix on the function name.

cburstedde commented 10 months ago

Wow, this was lots of work! I can see how this works technically.

I have one basic concern about the design. Please understand that this is not about the impressive technical skill that I can see in this draft PR.

Please let me discuss the files forestclaw?d.?, fclaw?d_convenience.?, and the upcoming fclaw?d_file.? where we are working on file-based checkpoint/restart. fclaw2d_to_3d.h is part of this set as well.

This set of files constitutes the core interface to p4est. It contains the historically first forestclaw files and is 100% written and maintained by UBonn (up to a few non-functional changes). They use the same 2d/3d logic as in p4est and are maximally consistent with the p4est library in general.

This PR not only touches these files but it also changes the deepest data structures directly connected with p4est. It constitutes a mix of the pure run-time switching desired for the higher layers of forestclaw and the p4est-style compile-time switching. It is genuinely cool, but it is also a new compromise of a scheme that obscures the initial design of these files.

The hierarchcy of header files usually has higher-level include lower-level files, as it is the case in most libraries including forestclaw. This PR turns this on its head, which is a deviation from the present relative simplicity of the library. It also includes forward typedefs which our files generally do not need.

Morever, the PR conflicts with both our upcoming 3D changes for the indirect exchange and the restart I/O. They will generally force us into a code design that is not our own and has no advantage for us in our daily work.

On a practical level, UBonn needs some safe space and technical wiggle room where we feel 100% at home, to develop freely according to our own expertise. These files are exactly that. They are a place for us to work without interference or risk of conflict, and where the responsibility for correctness is 100% with UBonn. This PR seriously endangers this principle.

So, given the above points, we may put our concern into one sentence:

Instead of changing us, would it be possible to wrap us?

For example, a file fclaw_domain.h may look like:

#include <forestclaw2d.h>
#include <forestclaw3d.h>

typedef struct fclaw_domain
  int dim;
  fclaw2d_domain_t *d2;
  fclaw3d_domain_t *d3;
}
fclaw_domain_t;

This will leave the UBonn files entirely alone, and the run-time switching happens just one file higher up the stack. Both @tim-griesbach and @cburstedde (@hannesbrandt is temporarily unavailable) feel rather stronly about keeping the core interface to p4est untouched. Hopefilly something along the lines of our above suggestion gives all of us what they wish for the structure of forestclaw.

scottaiton commented 10 months ago

Sure! I'll work on a branch that wraps the existing domain structures instead, and doesn't touch forestclaw2/3d.c.

cburstedde commented 9 months ago

Sure! I'll work on a branch that wraps the existing domain structures instead, and doesn't touch forestclaw2/3d.c.

Thanks, this sounds good!

You're welcome to help us out with the p4est interface files regardless, for example, to remove obsolete functions we'll be happy to integrate PRs of yours. We'll also be following up on issues raised about the forestclaw/convenience functionality and can discuss sensible PR content as needed.

Your construction of edge corner neighbors will likely be a valuable addition; we'll follow up separately.