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

style: modify parallel domain metadata with make build-proto #102

Conversation

nehalmamgain
Copy link
Contributor

@nehalmamgain nehalmamgain commented Aug 1, 2022

I made a virtual environment following pip install -r requirements-dev.txt
(Also tested this in the virtual environment following this.)

Although not documented in CONTRIBUTING (fixed now with #103), one of the former DGP maintainers asked to run make build-proto on this PR.

When running make build-proto, I saw a previous commit I made be formatted differently.
This PR commits the affected file with the output of make build-proto so that other users don't see a file unrelated to their PR also show up in their git status when contributing to this repository.


This change is Reviewable

@nehalmamgain
Copy link
Contributor Author

+reviewer:@tk-woven +reviewer:@kuanleetri

@nehalmamgain nehalmamgain force-pushed the feat/nehal/make-build-proto-for-pd-metadata branch 2 times, most recently from c6c9aa9 to 2ae837c Compare August 1, 2022 05:22
@nehalmamgain
Copy link
Contributor Author

I edited the commit message to follow linter expectations

@nehalmamgain nehalmamgain changed the title style: Modify PD metadata with make build-proto style: modify parallel domain metadata with make build-proto Aug 1, 2022
Copy link
Collaborator

@tk-woven tk-woven left a comment

Choose a reason for hiding this comment

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

Thanks! The generated file was built from the source tree as-is, no local changes?

We can probably extend DGP CI to build the protos and make sure everything us up to date as one of the checks.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kuanleetri)

@nehalmamgain
Copy link
Contributor Author

The generated file was built from the source tree as-is, no local changes?

Yup, exactly!

We can probably extend DGP CI to build the protos and make sure everything us up to date as one of the checks.

That sounds cool!

The CI failed. I don't believe the test is related to PD metadata though. Temporary failure (since it actually passed before; twice!)?
@tk-woven Sorry to bother you, but I don't have access to re-run this workflow without pushing a change. Could you please re-run it?

=================================== FAILURES ===================================
______________________ TestTorchUtilities.test_pose_utils ______________________

self = <tests.test_torch_extension_utils.TestTorchUtilities testMethod=test_pose_utils>

    def test_pose_utils(self):
        """Test pose class in dgp.utils.torch_extension.Pose and dgp.utils.torch_extension.QuaternionPose"""
    
        # Test pose transforms
        npposes = [NumpyPose(wxyz=pvec[:4], tvec=pvec[4:]) for pvec in self.pvecs]
        poses = [Pose(tf) for tf in self.tfs]
        qposes = [QuaternionPose.from_matrix(tf) for tf in self.tfs]
    
        # Test matrix composition of transformations
        final_pose_np = functools.reduce(lambda x, y: x @ y, [tf.numpy() for tf in self.tfs])
        final_pose_torch = functools.reduce(lambda x, y: x @ y, self.tfs)
        assert_true(np.allclose(final_pose_np, final_pose_torch.numpy(), atol=1e-6))
    
        # Test Pose manifold composition of transformations
        final_pose_NumpyPose = functools.reduce(lambda x, y: x * y, npposes)
        final_pose_Pose = functools.reduce(lambda x, y: x * y, poses)
        final_pose_QuaternionPose = functools.reduce(lambda x, y: x * y, qposes)
        assert_true(np.allclose(final_pose_np, final_pose_NumpyPose.matrix, atol=1e-6))
        assert_true(np.allclose(final_pose_np, final_pose_Pose.matrix.numpy(), atol=1e-6))
        assert_true(np.allclose(final_pose_np, final_pose_QuaternionPose.matrix.numpy(), atol=1e-6))
    
        def make_random_points(B=1, N=100):
            return torch.from_numpy(np.random.rand(B, 3, N)).type(torch.float)
    
        # Test single point cloud transformations for some implementations
        X = make_random_points()
        Xt_ = X[0].numpy()
        X_ = Xt_.T
    
        # Test point cloud transformations
        X1 = final_pose_Pose * X
        X2 = final_pose_QuaternionPose * X
        X3 = final_pose_NumpyPose * X_
        X4 = final_pose_np.dot(np.vstack([Xt_, np.ones((1, len(X_)))]))
    
>       assert_true(np.allclose(X1.numpy(), X2.numpy(), atol=1e-6))
E       AssertionError: False is not true

tests/test_torch_extension_utils.py:98: AssertionError
=========================== short test summary info ============================
FAILED tests/test_torch_extension_utils.py::TestTorchUtilities::test_pose_utils
=================== 1 failed, [57](https://github.com/TRI-ML/dgp/runs/7604952531?check_suite_focus=true#step:7:58) passed, 1 skipped in 41.[76](https://github.com/TRI-ML/dgp/runs/7604952531?check_suite_focus=true#step:7:77)s ===================

@tk-woven
Copy link
Collaborator

tk-woven commented Aug 1, 2022

Re-running now!

@nehalmamgain
Copy link
Contributor Author

Thank you, the coverage test passed. Please re-review 🙇‍♀️

Copy link
Collaborator

@tk-woven tk-woven left a comment

Choose a reason for hiding this comment

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

Thanks, :lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kuanleetri)

@nehalmamgain
Copy link
Contributor Author

I'll leave it up to you and @kuanleetri to merge this, thanks!

@tk-woven
Copy link
Collaborator

tk-woven commented Aug 1, 2022

Got it! I'm not very familiar with this process yet. @kuanleetri , I'd like your input if you have a moment. If some time passes and I can study this process for myself, I'll merge it on my own :)

@nehalmamgain
Copy link
Contributor Author

@tk-woven Btw could you please support here and check the merge process/reach out to @kuanleetri ?
It's not so important to get this PR merged but it's extremely important to merge #101 .

Copy link
Collaborator

@tk-woven tk-woven left a comment

Choose a reason for hiding this comment

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

I think we understand the merge process. What we're asking for here is Kuan's review since I'm not familiar with this part of the development. I will need to take some time to familiarize myself, but that has to wait for some other work I'm in the middle of. If you need to reach Kuan, please kindly try Slack (I have no special powers in this regard).

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kuanleetri)

tk-woven
tk-woven previously approved these changes Aug 3, 2022
Copy link
Collaborator

@tk-woven tk-woven left a comment

Choose a reason for hiding this comment

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

Nehal, I ran through the steps myself. Your work here LGTM! Can you please update your branch? Then when CI passes I can merge :)

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kuanleetri)

wadimkehl
wadimkehl previously approved these changes Aug 4, 2022
Copy link
Collaborator

@wadimkehl wadimkehl left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kuanleetri)

@nehalmamgain nehalmamgain dismissed stale reviews from wadimkehl and tk-woven via 4cce8c4 August 5, 2022 02:17
@nehalmamgain nehalmamgain force-pushed the feat/nehal/make-build-proto-for-pd-metadata branch from 2ae837c to 4cce8c4 Compare August 5, 2022 02:17
@nehalmamgain
Copy link
Contributor Author

Rebased off of master.

Copy link
Collaborator

@wadimkehl wadimkehl left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kuanleetri)

@wadimkehl wadimkehl merged commit 252e9b0 into TRI-ML:master Aug 5, 2022
@wadimkehl wadimkehl deleted the feat/nehal/make-build-proto-for-pd-metadata branch August 5, 2022 02:29
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.

3 participants