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

Chamfer distance, why is min_dist pow(2) and also why does no chunk size does not normalize ? #55

Open
JulienStanguennec-Leddartech opened this issue Nov 25, 2024 · 10 comments
Assignees

Comments

@JulienStanguennec-Leddartech
Copy link
Contributor

Hi,

When I compare with other definition and results provided by other implementation it seems that chamfer_distance does not seems to use .pow(2) while it does in Neurad implementation. Maybe it's more a question than a bug...

In the code for chamfer distance, when I replace the calculation for dist in _chamfer_dist to remove pow(2) (https://github.com/georghess/neurad-studio/blob/main/nerfstudio/utils/math.py#L770C1-L771C107)

This:

    def _chamfer_dist(source, target):
        dist = torch.cdist(source, target, p=2, compute_mode="use_mm_for_euclid_dist_if_necessary").pow(2)

Becomes this:

    def _chamfer_dist(source, target):
        dist = torch.cdist(source, target, p=2, compute_mode="use_mm_for_euclid_dist_if_necessary")

I get results that are inline with for example the point_cloud_utils (https://fwillims.info/point-cloud-utils/sections/shape_metrics/)

import point_cloud_utils as pcu
import torch
device =  torch.device('cuda' if torch.cuda.is_available() else 'cpu')
xyz1 = torch.rand(100000, 3, device=device)
xyz2 = xyz1 + 0.1
print(f"original_chamfer distance without normalization: {chamfer_distance_original(xyz1,xyz2, normalize_with_target=False, chunk_size=1000)}")
print(f"original_chamfer distance with normalization: {chamfer_distance_original(xyz1,xyz2, normalize_with_target=True, chunk_size=1000)}")
print(f"modified chamfer distance without normalization: {chamfer_distance_modified(xyz1,xyz2, normalize_with_target=False, chunk_size=1000)}")
print(f"modified chamfer distance with normalization: {chamfer_distance_modified(xyz1,xyz2, normalize_with_target=True, chunk_size=1000)}")
print(f"point_cloud_utils chamfer distance: {pcu.chamfer_distance(xyz1.float().cpu().numpy(),xyz2.float().cpu().numpy())}")

Gives:
original_chamfer distance without normalization: 269.503662109375
original_chamfer distance with normalization: 0.0026950370520353317
modified chamfer distance without normalization: 5048.60107421875
modified chamfer distance with normalization: 0.050485990941524506
point_cloud_utils chamfer distance: 0.05048602819442749

Question: is there a choice that drove you to use the square value of dist for chamfer distance calculation ?

Also, if chamfer distance is provided without chunk, normalization has no effect:- https://github.com/georghess/neurad-studio/blob/main/nerfstudio/utils/math.py#L777

    if chunk_size is None:
        min_dist_source_to_target, min_dist_target_to_source = _chamfer_dist(source_pc, target_pc)

The chamfer distance function is "partialled" with chunk_size=1000 and normalization=True so this function is never called without chunk_size=None. So probably that's why it did not came out.

@georghess georghess self-assigned this Nov 26, 2024
@georghess
Copy link
Owner

georghess commented Nov 26, 2024

Hi,

Thanks for bringing this up! As I remember it, we used the squared distance to have the same unit as for our depth metric (median squared depth error). We used median squared depth error mainly to have comparable numbers to UniSim. However, this should probably be clarified.

As for the normalization not having an impact when not chunking, that indeed looks like a bug. I'll fix it as soon as I have some spare time.

@anishmadan23
Copy link

anishmadan23 commented Nov 26, 2024

Given the squared median depth error and similarly for Chamfer distance, the numbers reported would be in square meters, and not in meters(as reported in the paper), right?

@georghess
Copy link
Owner

@anishmadan23 yes, I think our table caption might be missing a ^2 in that case

@anishmadan23
Copy link

Congrats on your SplatAD work, the results look great. I wanted to confirm if the SplatAD metrics (chamfer distance and median depth) are also computed using the squared term?

@georghess
Copy link
Owner

Thank you @anishmadan23! Yes, we use the same code to calculate the metrics so the numbers are comparable.

@anishmadan23
Copy link

gotcha! Thanks for the prompt response. When can we expect the code release for SplatAD btw?

@JulienStanguennec-Leddartech
Copy link
Contributor Author

@georghess, thanks for the clarification, it make sens. As anishmadan23 mentionned, congratulation for SplatAD ! A great contribution once again. Do you plan to release the code at some point ? Maybe we could add support for wod dataset :)

@carlinds
Copy link
Collaborator

@JulienStanguennec-Leddartech All CUDA-code will be released in our gsplat-fork once the paper has been peer reviewed and published.
The model itself will likely be released either as a model in neurad-studio, or as an external method (like we did with unisim).

@JulienStanguennec-Leddartech
Copy link
Contributor Author

Very good news ! Is it possible to know which conference it is ? As said above, if you want us to do the evaluation on waymo dataset, do not hesitate. As the rolling shutter direction is different in waymo dataset, it could be interesting to see if it has an impact.

@carlinds
Copy link
Collaborator

Unfortunately, the conference has some policies regarding that.
That would be interesting, we will reach out once the code is released :)

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

No branches or pull requests

4 participants