-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
use gsplat strategy interface to simplify splatfacto #3376
Conversation
Looks great! thanks for taking this on. Are you planning on adding support for MCMC? it would be nice to remove hyperparameters from SplatfactoConfig and replace them with a sort of StrategyConfig which can be swapped out for different strategies. This might require a gsplat PR to format them in a compatible way with tyro. I see that nerfstudio-project/gsplat#358 was closed without merging, is that change reflected inside the splatfacto hyperparams for gradient threhsolds now? |
Yes, but I am also open if someone else wish to take on this task. This PR is part of my own learning about the new strategy interface. This is indeed very convenient.
Yes, it takes some thoughts about how to change the SplatfactoConfig.
Yes, I retuned the parameter, I believe they are now very close. |
Since this PR removed the |
It seems
|
@kerrj Did we add a new test for building nerfstudio dockers? Not sure how to make it pass. |
@jkulhanek for Docker action, any idea? |
I think the error is that the docker build ran from a forked repo, but fork doesn’t have permission to run it. We should only run the docker action on the main repo - disable running it on forked repos I think. |
I've added a PR which should fix it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks reasonable to me!
As a thought for supporting MCMC the easiest way I can think of for adding this is just putting into the config:
strategy_mode: Literal["default", "mcmc"] = "default"
default_strategy: DefaultStrategy = dataclasses.field(default_factory=DefaultStrategy)
mcmc_strategy: MCMCStrategy = dataclasses.field(MCMCStrategy)
Or, if preventing breaking changes is more important than CLI flag organization:
strategy_mode: Literal["default", "mcmc"] = "default"
default_strategy: Annotated[DefaultStrategy, tyro.conf.arg(name="")] = dataclasses.field(
default_factory=DefaultStrategy
)
mcmc_strategy: MCMCStrategy = dataclasses.field(MCMCStrategy)
and then in the splatfacto constructor:
self.strategy = {
"default": self.config.default_strategy,
"mcmc": self.config.mcmc_strategy,
}[self.config.strategy_mode]
Hello, Thank you for your awesome work. I assume MCMC is not yet implemented. Do you have any update for when we could try this with nerfstudio? |
@abrahamezzeddine someone created a PR for mcmc. But I haven't gotten time to look at it. |
Hi, in this line, scene_scale is hard-coded to 1 nerfstudio/nerfstudio/models/splatfacto.py Line 332 in 516fd7c
From my understanding, scene_scale is the maximum of distance between average camera position and all camera positions. When I trace the code of nerfstudio's colmap dataparser, the default setting is to auto scale all the camera poses and normalized the camera positions to -1 and 1. This results in scene_scale = 1. However, in my application, I don't want to scale camera positions to -1 and 1. This results in scene_scale does not always = 1. |
@Sunnyhong0326 this is a known limitation. Ideally, we should pass scene scale from parser to the model in nerfstudio framework. Let me see what I can do here |
Given the latest updates of gsplat strategy interface, it is of interest to use the feature to benefit from multiple strategy implementations provided. The first step is to refactor existing splatfacto code meanwhile maintain the performance of current splafacto-big.
I spent some time debugging current gsplat implementation and found that we need to first merge this fix in order to reproduce the performance of current splatfacto-big. We don't necessarily need to merge the fix, this is just for producing baselines.
To benchmark simple_trainer.py in splatfacto configuration:
The above setting in simple_trainer is "almost" equivalent to splatfacto-big (post this PR merge) with two differences:
--strategy.*3d
parameters.Mipnerf360 bicycle
Baseline A (original Inria):
Baseline B (splatfacto-big in
main
):simple_trainer (using splatfacto configuration) + fix:
splatfacto-big (this PR) + fix:
splatfacto-big (this PR, densify_grad_thresh=0.0006):
splatfacto-big (this PR, densify_grad_thresh=0.0005):
Note that the default strategy is still a strong candidate if one chooses a proper configuration. The original Inria configuration is suboptimal.