stevenygd / PointFlow

PointFlow : 3D Point Cloud Generation with Continuous Normalizing Flows
https://www.guandaoyang.com/PointFlow/
MIT License
720 stars 101 forks source link

pairwise chamfer distance #20

Closed brjathu closed 3 years ago

brjathu commented 3 years ago

Hi,

When calculating MMD-CD score, in line,

https://github.com/stevenygd/PointFlow/blob/master/metrics/evaluation_metrics.py#L175

sample and reference points are interchanged. Is that correct?

stevenygd commented 3 years ago

Very good catch! I will push a fix. NOte that this shouldn't affect the results since CD and EMD are symmetrical. I've pushed a fix.

stevenygd commented 3 years ago

https://github.com/stevenygd/PointFlow/commit/fd00fce0daa37453a1469c669a83c44bfb54c44d

kazuto1011 commented 3 years ago

I think the previous state was correct since the distance matrix M_rs_cd was transposed .t() before calculating MMD.

https://github.com/stevenygd/PointFlow/blob/fd00fce0daa37453a1469c669a83c44bfb54c44d/metrics/evaluation_metrics.py#L177

https://github.com/stevenygd/PointFlow/blob/fd00fce0daa37453a1469c669a83c44bfb54c44d/metrics/evaluation_metrics.py#L157-L158

However, the current code calculates M_sr_cd in fact, not M_rs_cd. The pre-transposed matrix affects all the metrics.

https://github.com/stevenygd/PointFlow/blob/fd00fce0daa37453a1469c669a83c44bfb54c44d/metrics/evaluation_metrics.py#L175

Jiankai-Sun commented 3 years ago

@kazuto1011, I met the same problem. Can you successfully reproduce the result of Table 1 in the paper? Should we use M_rs_cd, M_rs_emd = _pairwise_EMDCD(ref_pcs, sample_pcs, batch_size, accelerated_cd=accelerated_cd) here instead of the current fix version that @stevenygd pushed?

kazuto1011 commented 3 years ago

Sorry I've not run PointFlow in any version. As far as I see evaluation_metrics.py, we should use the previous version. The order of tensor dimensions is inconsistent with the other functions.

aluo-x commented 3 years ago

@kazuto1011 @stevenygd Is there any conclusion on which version is correct?

Zhengxinyang commented 2 years ago

Based on the same observation with @kazuto1011, I also think the previous state was correct

kazuto1011 commented 2 years ago

Perhaps my first comment was a lack of words. CD and EMD are symmetric as the author said, but the distance matrix is not if the given pair of point clouds are different. Interchanging the pair by this fix would change the dim order and result in different distance matrices. MMD and COV depend on the dim of the distance matrix and are different from the original formulation, so I proposed to revert it back.

Quick check:

B, N = 2, 10
x = torch.rand(B, N, 3)
y = torch.rand(B, N, 3)

# The latest fix is like:
M_xy_cd, _ = _pairwise_EMD_CD_(sample_pcs=x, ref_pcs=y, batch_size=B)
M_yx_cd, _ = _pairwise_EMD_CD_(sample_pcs=y, ref_pcs=x, batch_size=B)

# The distance matrics are different
print(M_xy_cd == M_yx_cd)
# tensor([[ True, False],
#         [False,  True]])

# The scores are different
print(lgan_mmd_cov(M_xy_cd))
# {'lgan_mmd': tensor(0.1867), 'lgan_cov': tensor(1.), 'lgan_mmd_smp': tensor(0.2001)}

print(lgan_mmd_cov(M_yx_cd))
# {'lgan_mmd': tensor(0.2001), 'lgan_cov': tensor(0.5000), 'lgan_mmd_smp': tensor(0.1867)}