Skip to content

Conversation

@griffin28
Copy link
Collaborator

See AMR Spatial Field Implementation for a concrete example implementing this proposed AMR Spatial Field extension.

@johguenther johguenther changed the base branch from main to next_release December 6, 2023 17:59
@johguenther
Copy link
Collaborator

Regarding the cellWidth array:

  • the description (taken from VTK https://www.kitware.com/visualization-analysis-of-amr-datasets/) already makes it clear that the ratio between levels is integer
  • in the original Berger and Colella, “Local adaptive mesh refinement for shock hydrodynamics" paper the refinement ratio is even the same between all levels

    Grids will be refined in time as well as space, by the same mesh refinement ratio r, where $r=\Delta_{l-1}/\Delta_{l}$

  • CHOMBO is a bit more liberal, but still integer

    The integer $n^l_{ref}$ is the refinement ratio between level $l$ and $l+1$

  • another redundancy is that cellWidth[0] should be the same as spacing (isn't it?)

Thus proposed to replace cellWidth by an integer array refinementRatio (although the last element of that array will be
redundant / no used).

Regarding the name: AMR is used in the original paper and seems established. If there are other adaptive refinement schemes those will have a different name (like HTG).

@johguenther johguenther force-pushed the griffin/spatialfield_amr branch 2 times, most recently from 7aa9baf to e28bbc4 Compare January 17, 2025 14:07
@jeffamstutz
Copy link
Contributor

@ingowald any thoughts on @johguenther's response?

@ingowald
Copy link

ingowald commented Feb 7, 2025

Re "refinement levels are integers" - yes, they are. IIRC in practice they're always powers of two, in fact.

Re "always the same between levels" - in that paper they probably are, but in their definition they probably dont' have to be. IIRC their definition only stated the finer levels fully cover coarser level grid cells, that could in theory be any integer at any level. That said, IIRC the actual chombo file format doesn't actually talk much about levels at all, it just specifies bricks with dimension, cell width, and position.

Re cellWidth vs spacing. Yes, in pinricple they'll be the same value. It might still increase readablity to have both, because 'spacing' implies distance between cell centers, which - though the same value as that between cell boundaries - sounds like something else. Also, it is perfectly possible to interpret these as two different values: "cell width" as a per-brick logical cell width (ie, integer values that correspond to that brick's total refinement ratio), and "spacing" as a world-space spacing between finest-level grid cells, same as for structured volumes.

@johguenther
Copy link
Collaborator

Updates based on last discussions. Ready to be squashed and merged.

@johguenther johguenther force-pushed the griffin/spatialfield_amr branch from ba60e9d to 1dd60d9 Compare February 18, 2025 13:19
@jeffamstutz
Copy link
Contributor

jeffamstutz commented Apr 2, 2025

@johguenther is block.start defining an index into the current level of the grid, root level grid, the previous level?

@szellmann
Copy link

for block.start, the two coordinate systems that would make sense to me are:

  • "logical grid" coordinates of the block's actual level
  • "logical grid" coordinates of the finest level of the data set

where logical grid is the one you would get, hypothetically, if you projected all the cells on that level onto one big cartesian grid.

@griffin28
Copy link
Collaborator Author

@jeffamstutz in regards to the WG discussion on the correct extension name, I recommend (patch or block) structured AMR. My preference is

KHR_SPATIAL_FIELD_BLOCK_STRUCTURED_AMR
amrBlockStructured

instead of

KHR_SPATIAL_FIELD_PATCH_STRUCTURED_AMR
amrPatchStructured

However block and patch are generally used to mean the same thing. Here's some references that helped inform this recommendation:

VTK

VisIt

AMReX

@ingowald
Copy link

Concur on 'block structured' over 'patch structured'.
I had not even heard that term "patch structured" before your post, and we've never gotten any blow-back on any of our papers' use of the term 'block structured'.
From the links you sent it seems that amrex is fairly in the block structured camp, too; the visit related doc does indeed use patch-based, but is from 2013; and the kitware articles seem mixed (the first seems to mix both terms, the second uses blocks). If anything, the uses of the term "patch" seem to me to be using "patch based", not so much "patch structured", too.

@griffin28
Copy link
Collaborator Author

the uses of the term "patch" seem to me to be using "patch based", not so much "patch structured", too.

Yeah...patch-based seems to be used to imply patch structured as there's nothing preventing the patches from being unstructured. In the same vein, block-based has been used a few times to imply block structured.

@szellmann
Copy link

Currently implementing the latest changes on my side. One thing I'm debating that I'd really rather have block.data be a single linear array, that would be much more flexible in my opinion. As currently there is no way to just memcpy (or, gpuMempcy) the whole array at once, instead the device would always need to first wrangle the data into a linear array internally. And I suppose that virtually any somewhat optimized renderer would have to do that anyway.

I think the representation I would prefer is more SoA-like:

  • dataType (ANARYDataType, same for all cells)
  • block.start (1D array of vec3i)
  • block.size (1D array of vec3i)
  • block.level (1D array of int)
  • data (single array, offsets implicitly given by the former arrays/variables)

@jeffamstutz @johguenther @ingowald

@johguenther
Copy link
Collaborator

Interesting idea; OSPRay/VKL supports this for VDB volumes with the nodesPackedXXX arrays (but not yet for AMR).

  • dataType would not be needed, it can be deferred from datas element type
  • block.start and block.size could be merged back into block.bounds (array of INT32_BOX3)
  • implementations will likely precompute the offsets for quick random access to the blocks

@szellmann
Copy link

Yes exactly. An issue with the current draft is that it conflates what is raw data and what is topology. As the topology of the subgrids is in parts encoded in the array-of-array data structure holding the values. Currently it's only the subgrid sizes that are encoded in that data structure though, which seems arbitrary.

I would prefer a design that clearly distinguishes between data and topology, not only from an efficiency, but also from a software design perspective. This would also make the field type more similar to the unstructured field type, where data and topology are separated as well.

@szellmann
Copy link

I implemented that on my side and it simplifies things a lot.

This branch contains a patched version of TSD that implements these changes: https://github.com/szellmann/VisRTX/tree/amr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants