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 Metadata Handling to Cameras Collation Function #2001

Merged
merged 4 commits into from
May 27, 2023

Conversation

MingwuZheng
Copy link
Contributor

@MingwuZheng MingwuZheng commented May 25, 2023

This pull request addresses an issue in the nerfstudio_collate function where the metadata of Cameras objects is not properly accounted for during batch processing.

In the existing implementation, the metadata of Cameras objects is not included during collation, leading to the assignment of None to the metadata field. This may cause downstream issues for components that rely on this metadata for processing.

The proposed changes aim to ensure that the metadata of each Cameras object in a batch is properly collated. The updates involve the creation of a metadata dictionary that collates the metadata from each Cameras object in the batch. This dictionary is then passed to the Cameras constructor during batch assembly.

The changes ensure that all Cameras objects have the same metadata keys before proceeding with the collation. This ensures consistency across the batch and prevents potential errors from occurring due to mismatched or missing metadata keys.

@tancik
Copy link
Contributor

tancik commented May 25, 2023

Can you provide a PR description. Also look here for installing pre-commit to avoid these failing checks.

@MingwuZheng MingwuZheng changed the title Add metadata collate for cameras Add Metadata Handling to Cameras Collation Function May 25, 2023
@MingwuZheng
Copy link
Contributor Author

Can you provide a PR description. Also look here for installing pre-commit to avoid these failing checks.

My apologies for the initial oversight. I have now updated the PR description to provide more clarity on the changes being introduced.

Additionally, I would like to point out potential bugs in the current code. In the following section:

distortion_params=op(
    [
        cameras.distortion_params
        if cameras.distortion_params is not None
        else torch.zeros_like(cameras.distortion_params)
        for cameras in batch
    ],
    dim=0,
),
camera_type=op([cameras.camera_type for cameras in batch], dim=0),
times=torch.stack(
    [cameras.times if cameras.times is not None else -torch.ones_like(cameras.times) for cameras in batch],
    dim=0,
),

If cameras.distortion_params or cameras.times are None, the else clause attempts to execute torch.zeros_like(cameras.distortion_params) or -torch.ones_like(cameras.times) respectively. However, zeros_like and ones_like do not support None as an input. This could lead to an error during execution.

Regarding the failing checks, I'll ensure to install and run pre-commit as suggested in the contributing guidelines. Thank you for bringing this to my attention.

@MingwuZheng
Copy link
Contributor Author

Hey @tancik,
Just wanted to let you know that I've passed the tests and updated the PR. Could you take another look when you have a moment?

Copy link
Contributor

@tancik tancik left a comment

Choose a reason for hiding this comment

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

LGTM

@tancik tancik enabled auto-merge (squash) May 27, 2023 20:51
@tancik tancik merged commit ba08a8f into nerfstudio-project:main May 27, 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