Skip to content

Comments

Morton Order for supporting point transformer v3#217

Closed
TarzanZhao wants to merge 19 commits intomainfrom
morton_order
Closed

Morton Order for supporting point transformer v3#217
TarzanZhao wants to merge 19 commits intomainfrom
morton_order

Conversation

@TarzanZhao
Copy link
Collaborator

Point transformer v3 requires attention along the different serialization orders. This PR supports different z-order and transposed z-order. Next, I will implement hilbert order and transposed hilbert order to match with original ptv3 faithfully.

Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
@TarzanZhao TarzanZhao requested a review from a team as a code owner October 3, 2025 18:26
@TarzanZhao TarzanZhao added the core library Core fVDB library. i.e. anything in the _Cpp module (C++) or fvdb python module label Oct 3, 2025
Hexu Zhao added 2 commits October 6, 2025 16:30
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Copy link
Collaborator

@fwilliams fwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is failing due to style checks. Let's fix this.
We should think about naming here.

Let's also add unit tests in Python.

Copy link
Contributor

@blackencino blackencino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds a great deal to C++ that need not be there. Since the C++ build times dominate our iteration velocity, we should only port to C++ when strictly necessary - especially as it also gets in the way of torch's fusion capabilities.

All we need for the encoding functionality here are two functions, which do not need to be autograd function overloads but can just be regular methods that take and produce torch tensors. They are:

torch::Tensor morton(torch::Tensor const& ijk) {
   // for each index i, result[i] = morton(ijk[i[)
}

and the same thing for hilbert. These are generically useful functions that could be useful for a thousand other things beyond just point attention.

On the python side, we can then apply the encoding to the ijk of the grids or gridbatches directly. We can transpose those ijk to kji in python, and also use argsort for permutation. This minimizes compile time, C++ code support burden, and creates tools that are more flexibly useful.

Hexu Zhao added 6 commits October 17, 2025 02:45
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Hexu Zhao added 3 commits October 21, 2025 03:15
… _Cpp.pyi

Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
…lbert order implementation.

Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Copy link
Collaborator

@fwilliams fwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments:

I think the morton/hilbert/etc codes should be attributes on the grid in the same way that ijk is an attribute. i.e grid.morton_codes grid.hilbert_codes and variants for the transposed versions.

I also agree with Chris, that you can permute outside the grid.

So you have something like

grid = Grid.from_xxx(...)
sidecar = grid.inject_from_ijk(ijk_values, data)

morton_pmt = torch.argsort(grid.morton_codes)
sidecar_morton = sidecar[morton_pmt]

Copy link
Contributor

@blackencino blackencino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can go through with minimal documentation changes. I would like the offset, currently based on bounding box min, to be justified/explained with a comment in the code. Alternatively, you might choose to use the constant offset of (2^21-1), but either is ok, so long as it is explained. There's a documentation error in the permutation function that needs fixing.

@fwilliams
Copy link
Collaborator

Okay @blackencino and I discussed. Here's the conclusion:

  1. Rename encode_morton/hilbert/etc. to .morton/hilbert/etc. These are methods which take in an optional base offset:
    .i.e.
# Offset 
def hilbert(base_offset=None):

if called with None, use the -bbmin as the offset, otherwise, use the passed in offset
offset is added to the ijk values before they are transposed if they are transposed

2. Get rid of permute methods on the grid and in C++. Do this in Python by argsorting the space filling codes and indexing the sidecar.

Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Copy link
Contributor

@blackencino blackencino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, but please remove the permute methods

Hexu Zhao added 2 commits October 23, 2025 02:29
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Hexu Zhao added 2 commits October 28, 2025 10:02
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
@blackencino blackencino dismissed fwilliams’s stale review October 29, 2025 17:22

We covered the stuff, I just can't find the place to resolve it.

Signed-off-by: Hexu Zhao <hexuz@nvidia.com>
@blackencino
Copy link
Contributor

Merged in separate PR because of git commit signing issues.

@blackencino blackencino closed this Nov 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core library Core fVDB library. i.e. anything in the _Cpp module (C++) or fvdb python module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants