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

Verify method for blueprint nestset #285

Merged
merged 11 commits into from
Sep 11, 2018
Merged

Verify method for blueprint nestset #285

merged 11 commits into from
Sep 11, 2018

Conversation

nselliott
Copy link
Contributor

This adds a verify method for nestsets to describe the nesting of AMR domains as proposed in issue #255

@nselliott
Copy link
Contributor Author

@cyrush @xjrc Let me know what you think.

@xjrc
Copy link
Member

xjrc commented Apr 28, 2018

@nselliott: Thank you for adding this! After looking over your changes and thinking a bit more about the structure of the nestsets part of the schema, I think there are a few tweaks we can make to make it a bit more in line with the rest of the Blueprint components. I'm planning to discuss these changes with Cyrus on Monday and I'll post the results of our discussion here. Thanks again!

@xjrc xjrc added the feature label Apr 28, 2018
@xjrc xjrc added this to the 0.4.0 milestone Apr 28, 2018
@xjrc
Copy link
Member

xjrc commented May 11, 2018

@nselliott: I apologize for taking longer than anticipated to respond, but I was able to talk with @cyrush about this yesterday and here's a revised version of nestsets that we converged on:

{
  /* coordsets, topologies, etc. */
  "nestsets":
  {
    "nestset_name":  // shared name between processors
    {
      "association": "{vertex|element}",
      "topology": /* string identifying entry in "topologies" hierarchy */,
      "overlap":
      {
          "relation": "{parent|child}",
          "relation_id": /* id of the parent/child domain */,

          "ratio": [ /* refinement ratio {i_refine, j_refine, k_refine} */ ],
          "local": [ /* index range {i_min, j_min, k_min}, {i_max, j_max, k_max} for local overlap */ ],
          "remote": [ /* index range {i_min, j_min, k_min}, {i_max, j_max, k_max} for remote overlap */ ],
      }
    }
  }
}

The main motivation for all of these adjustments was to make each individual nestset more similar to an adjset, which we hope makes the Blueprint design more uniform. Please let me know @nselliott or @cyrush if you have any suggestions for adjustments (e.g. better names for nestset/overlap/relation and nestset/overlap/relation_id) and I'll get to work on revising this PR once we decide on a final design.

@rhornung67
Copy link
Member

@xjrc @cyrush @nselliott A few questions:

  • What is meant by relation and relation_id?
  • In SAMRAI (and other SAMR frameworks, I believe), there is one set of boxes on each level that defines the index space for that level; i.e., the "index space". Then, fields with different mesh centerings are stored in containers (e.g., SAMRAI PatchData objects) that map between the data centering and the index space to compute overlaps, etc. Is it reasonable to define one "nest set" for the index space and then use the data centering (associating) determine how individual fields overlap on the mesh? I guess I'm asking why it necessary to have a nest set for each centering?
  • For multiblock meshes, where each block has it's own i,j,k index space, doesn't there need to be some information about how blocks are stitched together at their boundaries (e.g., rotation and translation information)?

@nselliott
Copy link
Contributor Author

@rhornung67

What is meant by relation and relation_id?

This is describing something equivalent to overlap relationships between patches on different levels in SAMRAI. For the "relation" entry, a "parent" is a coarser domain/patch and a "child" is a finer domain/patch. The "relation_id" is a global integer identifier, unique on the hierarchy, of the parent or child domain. I would like different names for relation and relation_id, but I need to think about it.

I guess I'm asking why it necessary to have a nest set for each centering?

Under normal circumstances a user would choose one centering or the other and use that always. The "association" entry just identifies which centering is being described by the i-j-k index space in this particular instance. I would expect SAMRAI-based hierarchies to always use the element association, as that maps directly to SAMRAI's data structures, but some other code could have a SAMR hierarchy that defines its index spaces with a node/vertex centering.

For multiblock meshes, where each block has it's own i,j,k index space, doesn't there need to be some information about how blocks are stitched together at their boundaries (e.g., rotation and translation information)?

This particular addition to the blueprint concerns only coarse-fine overlap/nesting relationships within a block. I'm not sure if the blueprint already has a way to define multiblock boundaries, but it's outside the scope what the nestsets are intended to describe.

@rhornung67
Copy link
Member

Thanks @nselliott for the explanations. I'm curious what you folks come up with for the multiblock connectivity relationships because different codes use different conventions.

@cyrush
Copy link
Member

cyrush commented May 15, 2018

@nselliott @xjrc, how about domain_id instead of relation_id ? For adjsets we use neighbors as the name that groups the domain ids, but that won't work here.

@rhornung67 for domain abutment of unstructured meshes, we have something modeled on mfem's solution for how they describe communication surfaces. @bryujin and I have long had an idea for how to do structured mesh.

@xjrc
Copy link
Member

xjrc commented May 17, 2018

Here's a slightly revised version of the schema with improved names for relation and relation_id:

{
  /* coordsets, topologies, etc. */
  "nestsets":
  {
    "nestset_name":  // shared name between processors
    {
      "association": "{vertex|element}",
      "topology": /* string identifying entry in "topologies" hierarchy */,
      "overlap":
      {
          "remote_type": "{parent|child}",
          "remote_id": /* id of the parent/child domain */,

          "ratio": [ /* refinement ratio {i_refine, j_refine, k_refine} */ ],
          "local": [ /* index range {i_min, j_min, k_min}, {i_max, j_max, k_max} for local overlap */ ],
          "remote": [ /* index range {i_min, j_min, k_min}, {i_max, j_max, k_max} for remote overlap */ ],
      }
    }
  }
}

By changing the overlap relationship keys to have remote in them, we clearly establish that these values refer to the other value in this AMR association. Let me know what you all think!

Also, one other thing: it occurs to me that items in nestsets include ID information about the local and the remote domain while entries in adjsets only store local data. @cyrush, @nselliott: Could you refresh me on why the remote data needs to be stored for each nestsets entry? Is it the case that the remote may not define this information or something like that?

@cyrush
Copy link
Member

cyrush commented May 17, 2018

@xjrc I like the change toremote_id and remote_type.

You are right we were able to have less info in adjsets. I'll have to think more about what is different here, or maybe adjsets has less info than we need for general cases.

@rhornung67
Copy link
Member

@xjrc would it be better (i.e., more descriptive) to change 'local' to 'local_idx' and 'remote' to 'remote_idx' for the index ranges?

@xjrc
Copy link
Member

xjrc commented May 17, 2018

@rhornung67: That's a good idea, but I think if we're going to make those identifiers more descriptive we should go all the way (e.g. local_range and remote_range). If it turns out that we don't need both local and remote data (as is the case with nestsets), then range or window would be a good name for the local overlap space array I think.

@rhornung67
Copy link
Member

@xjrc whatever you guys decide is fine with me. Overall, I think that using descriptive names is preferable to relying on documentation.

@gzagaris
Copy link
Member

Another suggestion for your consideration, along the lines of @rhornung67's name suggestion: local_extent and remote_extent, which is pretty common nomenclature for block-structured AMR

@cyrush
Copy link
Member

cyrush commented May 17, 2018

Having the remote info is just used to identify which of the unique relationships this is. (You can have multiple relationships between two domains)

for adjsets, I believe the order of entries in the list matches across domains -- so we didn't need info to match them up.

I think that will be harder to do here.

Even if we can't use the order, we could impose an id for the relationship. Not sure if thats better than just including the extents

@cyrush
Copy link
Member

cyrush commented May 17, 2018

I like: local_extents and remote_extents

@nselliott
Copy link
Contributor Author

The remote extents can be computed by taking the local extents, multplying or dividing by the ratio, and making an adjustment if using the vertex association. If it's better to compute it on the fly rather than storing it here, we can do that.

@gzagaris
Copy link
Member

@nselliott That's exactly the conversation I was having with @cyrush earlier too

@cyrush
Copy link
Member

cyrush commented May 17, 2018

@nselliott, then that solves it -- we don't need the remote extents

@xjrc
Copy link
Member

xjrc commented May 17, 2018

Integrating @gzagaris's and @nselliott's feedback, here's the new nestsets schema:

{
  /* coordsets, topologies, etc. */
  "nestsets":
  {
    "nestset_name":  // shared name between processors
    {
      "association": "{vertex|element}",
      "topology": /* string identifying entry in "topologies" hierarchy */,
      "overlap":
      {
          "remote_type": "{parent|child}",
          "remote_id": /* id of the parent/child domain */,
          "ratio": [ /* refinement ratio {i_refine, j_refine, k_refine}; always relative to child */ ],
          "extents": [ /* index range {i_min, j_min, k_min}, {i_max, j_max, k_max} for local overlap */ ],
      }
    }
  }
}

@cyrush
Copy link
Member

cyrush commented May 18, 2018

I am backtracking on remote, mainly b/c it may not actually be remote, it could be a domain on the same mpi task.

What about: relationship = {parent|child} and domain_id ?

@xjrc
Copy link
Member

xjrc commented May 18, 2018

That's a good point Cyrus; I was actually getting the concepts a bit confused while thinking about it. I've created another revision with the following changes:

  • Changed overlap to nesting to better suit the convention established by the nestsets name.
  • Changed remote_type to just type to fall in line with the coordsets and topologies conventions (plus, reading nestset/nesting/type feels more natural).
  • Changed remote_id to domain_id for better descriptiveness. I'm tempted to change this again to something like partner instead because domain_id feels off; is there anything in the AMR vernacular to describe these pairs a la neighbors for adjacency sets?
{
  /* coordsets, topologies, etc. */
  "nestsets":
  {
    "nestset_name":  // shared name between processors
    {
      "association": "{vertex|element}",
      "topology": /* string identifying entry in "topologies" hierarchy */,
      "nesting":
      {
          "type": "{parent|child}", /* the type of nesting, relative to the local domain */
          "domain_id": /* id of the other domain in this nesting relationship */,
          "ratio": [ /* refinement ratio {i_refine, j_refine, k_refine} for nesting */ ],
          "extents": [ /* index range {i_min, j_min, k_min}, {i_max, j_max, k_max} for local nesting */ ],
      }
    }
  }
}

@gzagaris
Copy link
Member

domain_id seems reasonable to me, but, I am probably biased, since that's what we use in our code.

Perhaps another name for this would be patch_id. Folks in literature etc. often refer to what we call domains here as patches within an AMR patch hierarchy.

Another consideration is that the domain_id or patch_id (or whatever you end up calling this) is a unique across all AMR levels and all blocks. I think this requirement is fine -- it's easy for a host code to compute that -- but, I think it needs to be more explicitly stated in the documentation.

Alternatively, you could add some additional metadata attributes, for example, block_id, level_id to alleviate the requirement of a host-code providing a unique ID.

@cyrush
Copy link
Member

cyrush commented May 18, 2018

@gzagaris thanks for the input.

We are already using domain_id as a unique identifier in the state part of the mesh blueprint -- so I think thats sufficient, we don't need to add a new name.

I also believe the state will also have an optional level_id

@gzagaris
Copy link
Member

sounds good @cyrush, thanks.

1 similar comment
@gzagaris
Copy link
Member

sounds good @cyrush, thanks.

@cyrush
Copy link
Member

cyrush commented May 18, 2018

Here is a very basic example, likely some errors b/c I hand typed it, please pick apart :-)

bp_amr_simple

{
  "domain_0":
  {
    "state": 
    {
      "domain_id" : 0,
      "level_id": 0
    },
    "coordsets": 
    {
      "coords": 
      {
        "type": "uniform",
        "dims": { "i": 2, "j": 3},
        "origin": { "x": 0.0, "y": 0.0 },
        "spacing": {"dx": 1.0, "dy": 1.0}
      }
    },
    "topologies": 
    {
      "mesh": 
      {
        "type": "uniform",
        "coordset": "coords"
      }
    },
    "nestsets":
    {
      "nest_0":
      {
        "association": "element",
        "topology": "mesh",
        "nesting":
          {
            "type": "child", 
            "domain_id": 1,
            "ratio": [ 2, 2 ],  /* always rel to child , should we us i, j, k names ? */
            "extents": [ 1, 1 ,1, 1]   /* should we use explicit i_min, i_max,  etc to avoid confusion? */
          }
        }
     }
  },
  "domain_1":
  {
    "state": 
    {
      "domain_id" : 1,
      "level_id": 1
    },
    "coordsets": 
    {
      "coords": 
      {
        "type": "uniform",
        "dims": { "i": 2, "j": 2},
        "origin": { "x": 1.0, "y": 1.0 },
        "spacing": {"dx": 0.5, "dy": 0.5}
      }
    },
    "topologies": 
    {
      "mesh": 
      {
        "type": "uniform",
        "coordset": "coords"
      }
    },
    "nestsets":
    {
      "nest_0":
      {
        "association": "element",
        "topology": "mesh",
        "nesting":
          {
            "type": "parent", 
            "domain_id": 0,
            "ratio": [ 2, 2 ],   /* always rel to child , should we us i, j, k names ? */
            "extents": [ 0, 1 , 0, 1]   /* should we use explicit i_min, i_max,  etc to avoid confusion? */
          }
        }
     }
  }
}

@cyrush
Copy link
Member

cyrush commented May 18, 2018

I do think using i, j, and k for ratio would be better match to how we specify coordsets.

I also think we should consider the same for extents, b/c when its simply an array, I always have to detangle:

[i_min, i_max, j_min, j_max, k_min, k_max]

vs

[i_min, j_min, k_min, i_max, j_max, k_max]

We would of course pick one, but the point its not apparent from how the data is described.

@cyrush
Copy link
Member

cyrush commented May 29, 2018

There will not be one window per nestset.

@nselliott @gzagaris a named nestset actually represents a complete group of nesting info -- for example all of the local amr relationships for a given topology.

@cyrush
Copy link
Member

cyrush commented May 29, 2018

to clarify: within a named nestset, there will be as many windows as there are parent / child relationships for a given domain.

@nselliott
Copy link
Contributor Author

That's different from how I have understood this from the beginning, but we can roll with that.

Given that, I like @gzagaris suggestion in his most recent post to have "nestsets" hold up to two nestsets, one for the parent relationships and one for the child relationships. We can identify which is which either via a "type" label or a level number as shown in George's post. The rule would be that all windows within a nestset represent only parents or only children.

@gzagaris
Copy link
Member

gzagaris commented May 29, 2018

@cyrush , yes, I agree, you can have multiple "windows".

So, if you put both child & parent domains within the same named nestset, you cannot hoist up the level (as suggested by @xjrc) and probably must have it as an attribute of each named window?

Unless, perhaps if you do something like that:

"nestsets":
{
  "nestset_name": /* parent domains  */
  {
    "association": "{vertex|element}",
    "topology": "topology_name",

    "parent_windows": /* equivalent to 'groups' member of 'adjsets' */
    {
        "level": <N-1>
        "window_0": /* shared name between domains (e.g. window0 for quadrant 0) */
        {
            "domain_id": /* **NAME PENDING** ; id of the other domain in this nesting relationship */,
            "ratio": { /* mcarray of logical extents per-dimension, e.g. { i: 1, j: 1, k: 1 } */ },
            "origin": { /* mcarray indicating logical origin in topology, e.g. { i: 1, j: 1, k: 1 } */ },
            "dims": { /* mcarray indicating the span of elements in topology in logical space, e.g. { i: 1, j: 1, k: 1 } */ },
        "window_1": /* shared name between domains (e.g. window0 for quadrant 0) */
        {
            "domain_id": /* **NAME PENDING** ; id of the other domain in this nesting relationship */,
            "ratio": { /* mcarray of logical extents per-dimension, e.g. { i: 1, j: 1, k: 1 } */ },
            "origin": { /* mcarray indicating logical origin in topology, e.g. { i: 1, j: 1, k: 1 } */ },
            "dims": { /* mcarray indicating the span of elements in topology in logical space, e.g. { i: 1, j: 1, k: 1 } */ },
        }

   "child_windows": /* equivalent to 'groups' member of 'adjsets' */
    {
        "level": <N+1>
        "window_0": /* shared name between domains (e.g. window0 for quadrant 0) */
        {
            "domain_id": /* **NAME PENDING** ; id of the other domain in this nesting relationship */,
            "ratio": { /* mcarray of logical extents per-dimension, e.g. { i: 1, j: 1, k: 1 } */ },
            "origin": { /* mcarray indicating logical origin in topology, e.g. { i: 1, j: 1, k: 1 } */ },
            "dims": { /* mcarray indicating the span of elements in topology in logical space, e.g. { i: 1, j: 1, k: 1 } */ },
        }
    }
}

@xjrc
Copy link
Member

xjrc commented May 29, 2018

@gzagaris: That's close to what I was thinking, but I don't even think the separation of parent/child relationships is necessary. In particular, what I was suggesting was to store level at the same level as association and to have parent/child relationships be implied by comparing level values. When applying this to Cyrus' example above, you get the following:

{
  "domain_0":
  {
    "state": 
    {
      "domain_id" : 0
    },
    "coordsets": 
    {
      "coords": 
      {
        "type": "uniform",
        "dims": { "i": 2, "j": 3 },
        "origin": { "x": 0.0, "y": 0.0 },
        "spacing": {"dx": 1.0, "dy": 1.0}
      }
    },
    "topologies": 
    {
      "mesh": 
      {
        "type": "uniform",
        "coordset": "coords"
      }
    },
    "nestsets":
    {
      "nest_0":
      {
        "association": "element",
        "topology": "mesh",
        "level": 0,
        "windows":
        {
          "window_0":
          {
            "domain_id": 1,
            "ratio": { "i": 2, "j": 2 },
            "origin": { "i": 1, "j": 1 },
            "dims": { "i": 1, "j": 1 },
          }
        }
      }
    }
  },
  "domain_1":
  {
    "state": 
    {
      "domain_id" : 1
    },
    "coordsets": 
    {
      "coords": 
      {
        "type": "uniform",
        "dims": { "i": 2, "j": 2},
        "origin": { "x": 1.0, "y": 1.0 },
        "spacing": {"dx": 0.5, "dy": 0.5}
      }
    },
    "topologies": 
    {
      "mesh": 
      {
        "type": "uniform",
        "coordset": "coords"
      }
    },
    "nestsets":
    {
      "nest_0":
      {
        "association": "element",
        "topology": "mesh",
        "level": 1,
        "windows":
        {
          "window_0":
          {
            "domain_id": 0,
            "ratio": { "i": 2, "j": 2 },
            "origin": { "i": 0, "j": 0 },
            "dims": { "i": 2, "j": 2 },
          }
        }
     }
  }
}

If a client is going to inspect the nestset relationship between domains 0 and 1, they'll find that domain 0's level is lower than domain 1's and thus must be the parent between the two. Assuming that these AMR relationships can always be modeled as a tree (DAG), then the maximum length path from the root (supersource) would dictate level and could always be used to determine relationships. The problem with this scheme is that this relationship information isn't readily available, which may be undesirable (e.g. when we want to navigate to only child windows at a particular level).

@cyrush
Copy link
Member

cyrush commented May 29, 2018

the info we need is the level of the remote, so level will need to be part of each window spec, not hoisted, unless we group like George suggests.

I think replacing "type" with "level", for each window may work.

@gzagaris
Copy link
Member

@xjrc , I think you need something like state/level to store the level of a given domain and then nestsets/nest_<i>/level stores the level of the neighboring domain(s)

@gzagaris
Copy link
Member

@cyrush , @xjrc , the refinement ratio may be another thing that can be hoisted up, instead of storing it for each named window. In theory, the refinement ratio can change, but, in practice, I just haven't seen anyone run like that -- it is "fairly" constant across all levels for a given run.

@cyrush
Copy link
Member

cyrush commented May 31, 2018

@gzagaris, I think there is limited value to hoisting things that potentially do vary a the window level (such as the level and the ratios) If its even possible they could vary there -- to process the info you would have to always look in two places just to be sure.

@nselliott
Copy link
Contributor Author

I'm working on an implementation inside SAMRAI to output all of this stuff, but I think we still need to nail down a consensus for the format. The main thing I see yet to be determined is how to identify the parent/child relationships.

The ideas I see are:

  • A string label: "type": "{parent|child}"
  • A level number integer, could be stored in each window or higher in the nestset
  • Grouping parents and children into separate nestsets

Any of these I could implement without much trouble, so it's a matter of coming to a conclusion on what the group wants.

@cyrush
Copy link
Member

cyrush commented Jun 7, 2018

@nselliott
The first two are still up in the air, as for the 3rd -- we wont group parents and children into separate nestsets. The nestset will have a complete set of entries for the local domains of a topology.

As for the first:
I am leaning towards where we started at the beginning:

"parent": domain_id
or
"child": domain_id

Hopefully changing small details won't be too hard once you have worked out the bigger details.

* Made some minor improvements to the automated structured AMR schema generation code.
@xjrc
Copy link
Member

xjrc commented Jun 7, 2018

@nselliott: Could you give me permission to push to this branch on your repository? I have an update that includes fixes and a complete set of test cases. Once I've pushed these changes, I think we're only a minor adjustment or two away from this being finalized.

EDIT: Disregard the above request; it turns out the permissions problem was related to me pushing to the wrong branch.

@nselliott
Copy link
Contributor Author

@xjrc Thanks. Everyone feel free to make useful changes.

@cyrush
Copy link
Member

cyrush commented Sep 11, 2018

@xjrc @nselliott , I think we have converged on this & this PR is ready.
If something comes up we can resolve in a new PR.

@xjrc, it looks like there is a small conflict, can you resolve that and then we can merge?

@xjrc
Copy link
Member

xjrc commented Sep 11, 2018

@cyrush: Done; merge will be ready assuming all continuous integration checks pass.

@cyrush
Copy link
Member

cyrush commented Sep 11, 2018

thanks @xjrc !

@cyrush cyrush merged commit 5230ebe into LLNL:master Sep 11, 2018
@nselliott
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants