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

Potential mathematical error for calculating con_o.y's gradient in backward process #64

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yangjiheng
Copy link

First of all, thank you for this wonderful project and elegant implementation which is both educative and precise from all aspects! This is really one of the world changing project in the history of computer science.

It came across to me that the constant factor for con_o.y in backward process is not consistent with forward process mathematically. But I'm not that sure about this, so I want to point this out and try to clear my doubts. Please help check whether this makes sense.

In forward process:

float power = -0.5f * (con_o.x * d.x * d.x + con_o.z * d.y * d.y) - con_o.y * d.x * d.y;

Then in the backward process, dL_dconic2D.y is calculated with:

atomicAdd(&dL_dconic2D[global_id].y, -0.5f * gdx * d.y * dL_dG);

To be consistent to the forward process, the constant factor of the gradient for con_o.y should be -1.0f, but the current implementation used -0.5f. So I wonder whether this is a minor mistake.

In this case, it should be implemented as:

atomicAdd(&dL_dconic2D[global_id].y, -1.0f * gdx * d.y * dL_dG);

I ran some experiments with updated factor and it seems OK quality-wise (not much improvement, but also not dropping in PSNR). So if this is a mathematical error, fixing this should make it more precise. Although it's highly possible that my understanding is not correct. Hence this PR to reach out to you guys to check it out. It could totally be some kind of technique or calculation error on my side that I'm not aware of.

Thanks again for sharing this with the rest of the us!

…onsistent with forward process. Please check whether I'm correct
@dtjun
Copy link

dtjun commented Oct 29, 2024

IMG_20241029_201433.jpg

试着推了一下

@yangjiheng
Copy link
Author

IMG_20241029_201433.jpg

试着推了一下

Exactly, so it makes sense for x and z to be -0.5, but for y, it should be -1.0 right?

@dtjun
Copy link

dtjun commented Nov 3, 2024

IMG_20241029_201433.jpg
试着推了一下

Exactly, so it makes sense for x and z to be -0.5, but for y, it should be -1.0 right?

I think so

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.

2 participants