Fix bug in pixel sampler

[carlinds]

Current sampling will oversample the first (num_images - 1) images, and might return a negative number of rays for the last image. The proposed fix finds an optimal ratio of rays per image such that the constraint of always being divisible by 2 is still satisfied.

[jb-ye]

Would it be simpler if we change the loop for i in range(num_images): to a random order in each sample call?

[carlinds]

It would still fail due to getting a negative number of rays for the last image you loop over, no? Or do you propose to skip the last image in that case? Then you would end up with more rays than expected for the batch, which doesn't sound like desired behavior. Perhaps I misunderstood your suggestion. :)

[jb-ye]

My read of the current code is current sampling will undersample the first (num_images - 1) images, and might return a number greater than num_rays_per_batch // num_images for the last image. It doesn't seem like a bug though.

The main issue is this line:

        if num_rays_in_batch % 2 != 0:
            num_rays_in_batch += 1

which would cause the last image number to be negative. What if we just remove this part of code? Why the num_rays_in_batch must be even?

I was thinking oversample last image is problematic, we can do a shuffling of the image indices each time we sample rays.

[carlinds]

According to the latest fix to this file, num_rays_in_batch needs to be divisible by 2 for depth nerfacto to work: #2939

Current code will oversample the first (num_images - 1) images, no? For example, let's say you have specified 4096 rays per batch and have 240 images. Ideally, you'd like to sample 17.067 rays per image (which is impossible of course). The current code will instead sample 18 rays per image. The first 239 images will then add up to 4306 rays, which is more than we desire and thus result in a negative number of rays on the last image.

If you instead wish to undersample the first (num_images - 1), i.e by doing

        if num_rays_in_batch % 2 != 0:
            num_rays_in_batch -= 1

you would end up oversampling the last image. When the number of images grow, so will the number of rays for the last image.

[jb-ye]

@carlinds Thanks for the explanation! now I have better understanding of what this PR solves. I have some additional comments (see above).

[carlinds]

@jb-ye Which comments are you referring to? I couldn't find any comments on the commit.

[jb-ye]

@jb-ye Which comments are you referring to? I couldn't find any comments on the commit.

I have inline comments here:
"""
Does this guarantee num_rays_per_image[-1] is an even number? (what if num_rays_per_batch is a odd number?)

Also could you refactor this section of code to a separate function?
"""

you also need to run the formatter to pass core tests and make the PR ready to merge.

ruff nerfstudio/data/pixel_samplers.py

[carlinds]

Do we need to support the case where num_rays_per_batch is odd? Naturally, it can't be divided into even chunks so we would either need to handle it by changing it to the closest even number or simply assert and inform the user. As it's a user-specified argument I would prefer an assert.