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

Fix benchmarking: apply get_background_color in renderer and set profiler to none #2397

Merged
merged 3 commits into from
Sep 11, 2023

Conversation

Mxbonn
Copy link
Contributor

@Mxbonn Mxbonn commented Sep 5, 2023

Running ./nerfstudio/scripts/benchmarking/launch_train_blender.sh -m nerfacto failed because of two errors:
logging.enable-profiler is does not exist and the background_color can be a string like white.

This pull request fixes both issues.
Note: the original code mentioned: # This case must be before the others or the override is not properly applied
I moved that case to become the final one, but as they are all still separated by an if - else clause, I don't think it breaks any assumptions.

A further comment related to benchmarking: Would it be possible to add benchmark results somewhere in this repo/documentation along with the last commit/stable release the benchmark was run on? This would let anyone use benchmark results without having to recompute them, as long as they are on the same code base.

@Mxbonn
Copy link
Contributor Author

Mxbonn commented Sep 5, 2023

Benchmark results:
image

@@ -101,19 +101,15 @@ def combine_rgb(
else:
comp_rgb = torch.sum(weights * rgb, dim=-2)
accumulated_weight = torch.sum(weights, dim=-2)
if BACKGROUND_COLOR_OVERRIDE is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @Mxbonn!

Can you double-check this logic? I think the override should be applied even when the background color is set to random or last_sample, which doesn't seem like it would happen if you remove this first if statement.

Adding the else at the end makes sense to me though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @brentyi, you're right. It should indeed still be applied in case of an override.
However, the current way to do it seems a bit awkward to me since get_background_color serves both to override the background color and to convert it to RGB. But you cannot just simply pass background color random or last_sample because this will fail the assertion in case there is no override. So you need to do the check for override before calling the method. But if you're doing the override check anyway, then you should immediately apply it instead of calling the get_background_color.
Anyways I made a change to the code which should be correct now.

Copy link
Collaborator

@brentyi brentyi left a comment

Choose a reason for hiding this comment

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

Thanks @Mxbonn!!

@brentyi brentyi enabled auto-merge (squash) September 11, 2023 17:54
@brentyi brentyi merged commit cc98fb6 into nerfstudio-project:main Sep 11, 2023
4 checks passed
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.

None yet

2 participants