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

Add kwargs handling for patchpixelsampler patch size #2622

Merged
merged 5 commits into from May 8, 2024
Merged

Conversation

chungmin99
Copy link
Contributor

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:

return PatchPixelSamplerConfig().setup(

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...

@brentyi
Copy link
Collaborator

brentyi commented Nov 19, 2023

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()

@chungmin99
Copy link
Contributor Author

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.

pixel_sampler: PixelSamplerConfig = PixelSamplerConfig()

@chungmin99 chungmin99 merged commit c17d1d7 into main May 8, 2024
2 checks passed
@chungmin99 chungmin99 deleted the cmk/patch-fix branch May 8, 2024 17:45
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

5 participants