nerfstudio icon indicating copy to clipboard operation
nerfstudio copied to clipboard

Add kwargs handling for patchpixelsampler patch size

Open chungmin99 opened this issue 2 years ago • 2 comments

PatchPixelSamplerConfig has a patch_size item, but the current PatchPixelSampler init function doesn't override the value of patch_size with the corresponding kwargs value.

This is particularly problematic, because PatchPixelSamplerConfig is generated within the datamanager: https://github.com/nerfstudio-project/nerfstudio/blob/9528a3f717fe28a6c6500199b5b37d4f2c6e10ca/nerfstudio/data/datamanagers/base_datamanager.py#L472

This PR simply adds the __init__ function for the sampler and adds a self.config.patch_size = self.kwargs.get(....

As an aside, it would be good if this override could happen automatically...

chungmin99 avatar Nov 19 '23 01:11 chungmin99

Possibly dumb question: in the linked snippet, why are the kwargs used/needed at all?

Like instead of:

            return PatchPixelSamplerConfig().setup(
                patch_size=self.config.patch_size, num_rays_per_batch=num_rays_per_batch
            )

Would this work?

            return PatchPixelSamplerConfig(
                patch_size=self.config.patch_size,
                num_rays_per_batch=num_rays_per_batch,
            ).setup()

brentyi avatar Nov 19 '23 02:11 brentyi

I believe the latter should work.

On another note (maybe address in this PR or in some future), perhaps it would be better to have people manually change the datamanager's pixel_sampler to PatchPixelSampler in the method configs, instead of automatically generating it under the hood when patch_size > 1.

https://github.com/nerfstudio-project/nerfstudio/blob/9528a3f717fe28a6c6500199b5b37d4f2c6e10ca/nerfstudio/data/datamanagers/base_datamanager.py#L348

chungmin99 avatar Nov 19 '23 21:11 chungmin99