Allow constant inputs to cost functions to be passed as floats #150

jeffin07 · 2022-04-10T09:24:14Z

Motivation and Context

To close #38

How Has This Been Tested

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

jeffin07 · 2022-04-10T09:27:08Z

@luisenp @mhmukadam I have added some changes to collisions.py and double_integrator.py. Tests not included. I will add test once the feature is finalized.Also i do see lot of variables in other cost functions,Could you please tell me which one are elligible for this change ?

theseus/embodied/collision/collision.py

luisenp

This is looking good! Some tweaks needed but mostly easy fixes and unit tests. The only other variable I've seen so far where this applies is eff_radius in EffectorObjectContactPlanar. Poses and sdf variables (origin, cell size, data) are multidimensional, so no need to change those.

theseus/embodied/collision/collision.py

luisenp · 2022-04-12T20:01:19Z

theseus/embodied/motionmodel/double_integrator.py

+                dt = torch.tensor(dt)
+            self.dt = Variable(dt)
+        else:
+            self.dt = dt


After the ndim check below, we can add dt.data = self.dt.data.view(-1, 1), to make sure we have a batch dimension as well, as discussed above.

luisenp · 2022-04-12T20:02:00Z

theseus/embodied/motionmodel/double_integrator.py

        name: Optional[str] = None,
    ):
        super().__init__(name=name)
        if not isinstance(dt, Variable):
+            if not isinstance(dt, torch.Tensor):
+                dt = torch.tensor(dt)


I have added a fix for this class in #157, make sure you merge those changes as well.

luisenp · 2022-04-12T20:02:40Z

theseus/embodied/motionmodel/double_integrator.py

+                dt = torch.tensor(dt)
+            self.dt = Variable(dt)
+        else:
+            self.dt = dt
        super().__init__(pose1, vel1, pose2, vel2, dt, cost_weight, name=name)


Similar comment about maintaining a batch shape here.

jeffin07 · 2022-04-13T12:12:26Z

@luisenp Added the changes. Please review.Also do you have any suggestions for unit test ?

luisenp

LGTM! For unit tests, just constructing all of these classes with the different types of inputs, and checking that the corresponding member is a variable of the expected type and shape should be sufficient. Thanks!!

jeffin07 · 2022-04-13T14:57:40Z

@luisenp Added unit test. Can you please take a look ?

luisenp

Looks good, almost there! Once the code is finished, I'll also run some tests on my side before confirming that we can merge. Thanks!

luisenp · 2022-04-15T14:54:43Z

theseus/embodied/collision/tests/test_eff_obj_contact.py

+            obj, eff, cost_weight, origin, sdf_data, cell_size, eff_radius
+        )
+
+        assert isinstance(cost_function.eff_radius, Variable)


Here we can check that cost_function.eff_radius is eff_radius.

You can delete the line above that has the isinstance check now.

luisenp · 2022-04-15T14:55:24Z

theseus/embodied/collision/tests/test_eff_obj_contact.py

+        )
+
+        assert isinstance(cost_function.eff_radius, Variable)
+


Here we can add another check that cost_function.eff_radius.data is eff_radius_t.

@luisenp While assert cost_function.eff_radius.data is eff_radius_t gives error as one is tensor and other is th.Variable .Is it better to use assert torch.allclose(cost_function.eff_radius.data,eff_radius_t) ?

I'm a bit confused. Shouldn't both cost_function.eff_radius.data (notice, .data) and eff_radius_t be both torch tensors?

@luisenp when i try to do assert cost_function.eff_radius.data is eff_radius_t i get this error which expained above

> assert cost_function.eff_radius.data is eff_radius_t E assert tensor([[0.4160]], dtype=torch.float64) is tensor([[0.4160]], dtype=torch.float64) E + where tensor([[0.4160]], dtype=torch.float64) = Variable(data=tensor([[0.4160]], dtype=torch.float64), name=Variable__680).data E + where Variable(data=tensor([[0.4160]], dtype=torch.float64), name=Variable__680) = <theseus.embodied.collision.eff_obj_contact.EffectorObjectContactPlanar object at 0x7f813704edd0>.eff_radius theseus/embodied/collision/tests/test_eff_obj_contact.py:180: AssertionError

Ah, right, it's because of the view(-1, 1) we added. In this case, let's stick to compare that they are close in value as you had initially.

@luisenp Then can you take a look at PR. I've already committed the changes

luisenp · 2022-04-15T14:57:07Z

theseus/embodied/collision/tests/test_eff_obj_contact.py

+            obj, eff, cost_weight, origin, sdf_data, cell_size, eff_radius_f
+        )
+
+        assert isinstance(cost_function.eff_radius, Variable)


Here we can first set up some random value for eff_radius_f and then add an additional check that np.isclose(cost_function.eff_radius.data.item(), eff_radius_f).

@luisenp
we're already giving a random value to eff_radius_f=1.0
So is this enough assert np.isclose(cost_function.eff_radius.data.item(), eff_radius_f)

By random I mean sampling a new value every iteration rather than hardcoding to 1.0.

luisenp · 2022-04-15T14:57:56Z

theseus/embodied/motionmodel/tests/test_double_integrator.py

@@ -60,6 +61,30 @@ def test_gp_motion_model_cost_weight_copy():
    )


+def test_gp_motion_model_variable_type():


Add similar modifications to the test here, as mentioned above.

luisenp · 2022-04-15T14:58:24Z

theseus/embodied/motionmodel/tests/test_double_integrator.py

+            assert isinstance(cost_weight.Qc_inv, Variable)
+            assert isinstance(cost_weight.dt, Variable)
+
+            q_inv_v = Variable(q_inv)


We don't need the second check for q_inv_v.

jeffin07 · 2022-04-19T18:15:13Z

@luisenp Added changes please review

jeffin07 · 2022-04-20T13:52:32Z

@luisenp @mhmukadam I'm unable to run tests after fetching latest changes from upstream/main. When running pytest theseus/ gives the following output. Any idea what I've done wrong ?

ImportError while importing test module '/theseus/theseus/optimizer/tests/test_sparse_linearization.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/local/lib/python3.7/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
theseus/optimizer/tests/test_sparse_linearization.py:9: in <module>
    import theseus as th
theseus/__init__.py:71: in <module>
    import theseus.embodied as eb
theseus/embodied/__init__.py:6: in <module>
    from .collision import Collision2D, EffectorObjectContactPlanar, SignedDistanceField2D
theseus/embodied/collision/__init__.py:6: in <module>
    from .collision import Collision2D
theseus/embodied/collision/collision.py:11: in <module>
    from theseus.embodied.kinematics import IdentityModel, KinematicsModel
theseus/embodied/kinematics/__init__.py:6: in <module>
    from .kinematics_model import IdentityModel, KinematicsModel, UrdfRobotModel
theseus/embodied/kinematics/kinematics_model.py:9: in <module>
    import differentiable_robot_model as drm
E   ModuleNotFoundError: No module named 'differentiable_robot_model'

This keeps repating for every file

luisenp · 2022-04-20T14:05:32Z

We merged a new component that has an additional requirement. Can you try pip installing this?

luisenp · 2022-04-20T16:41:01Z

theseus/embodied/motionmodel/tests/test_double_integrator.py

            cost_weight = th.eb.GPCostWeight(q_inv_v, dt_f)
-            assert isinstance(cost_weight.Qc_inv, Variable)
            assert isinstance(cost_weight.dt, Variable)


Sorry, didn't think of this before, but we should also check that cost_weight.dt has a batch dimension. And similar for eff_radius_t in the other cost function. We are also missing the check np.isclose(cost_function.eff_radius.data.item(), eff_radius_f) (and similar for dt).

@jeffin07 Missing the np.allclose() check for eff_radius_f and also checking that the batch dimension is present.

luisenp

Left two more comments.

jeffin07 · 2022-04-20T17:08:27Z

@luisenp Updated unit tests

jeffin07 · 2022-04-21T10:18:46Z

@luisenp Updated the changes.Please review

luisenp

Sorry for the delay, got busy with other PRs and forgot this was waiting for review. LGTM!

luisenp · 2022-04-26T15:56:29Z

Thanks a lot for another great contribution, @jeffin07 :)

theseus/embodied/collision/tests/test_eff_obj_contact.py

mhmukadam

Looks good! Thanks.

jeffin07 · 2022-04-27T03:32:49Z

@luisenp @mhmukadam Updated the changes. Please review

…ookresearch#150) * collision and double_integrator variable change * unit test

collision and double_integrator variable change

8c65a5c

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 10, 2022

mhmukadam requested review from mhmukadam and luisenp April 10, 2022 21:43

mhmukadam assigned jeffin07 Apr 10, 2022

mhmukadam added the refactor Refactor library components label Apr 10, 2022

mhmukadam reviewed Apr 10, 2022

View reviewed changes

theseus/embodied/collision/collision.py Show resolved Hide resolved

luisenp requested changes Apr 12, 2022

View reviewed changes

jeffin07 added 2 commits April 13, 2022 09:58

upstream-merge

63bd33b

review changes for feature

bee7bc5

luisenp requested changes Apr 13, 2022

View reviewed changes

unit test

67cbaa5

luisenp requested changes Apr 15, 2022

View reviewed changes

jeffin07 added 2 commits April 19, 2022 17:55

upstream merge

d9b3e6f

changes in unit test after review

c4cc96b

luisenp reviewed Apr 20, 2022

View reviewed changes

luisenp requested changes Apr 20, 2022

View reviewed changes

changes after review in unit test

e3e3be3

review chnages in unit test

9c6d6af

luisenp approved these changes Apr 26, 2022

View reviewed changes

mhmukadam reviewed Apr 26, 2022

View reviewed changes

theseus/embodied/collision/tests/test_eff_obj_contact.py Outdated Show resolved Hide resolved

mhmukadam approved these changes Apr 26, 2022

View reviewed changes

jeffin07 added 2 commits April 27, 2022 03:21

upstream-merge

7ddfff2

changes after review

fcb87d1

mhmukadam merged commit 3498bbd into facebookresearch:main Apr 27, 2022

suddhu pushed a commit to suddhu/theseus that referenced this pull request Jan 21, 2023

Allow constant inputs to cost functions to be passed as floats (faceb…

fedfe91

…ookresearch#150) * collision and double_integrator variable change * unit test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow constant inputs to cost functions to be passed as floats #150

Allow constant inputs to cost functions to be passed as floats #150

jeffin07 commented Apr 10, 2022

jeffin07 commented Apr 10, 2022

luisenp left a comment

luisenp Apr 12, 2022

luisenp Apr 12, 2022

luisenp Apr 12, 2022

jeffin07 commented Apr 13, 2022

luisenp left a comment

jeffin07 commented Apr 13, 2022

luisenp left a comment

luisenp Apr 15, 2022

luisenp Apr 20, 2022

luisenp Apr 15, 2022

jeffin07 Apr 19, 2022

luisenp Apr 19, 2022

jeffin07 Apr 20, 2022

luisenp Apr 20, 2022

jeffin07 Apr 20, 2022

luisenp Apr 15, 2022

jeffin07 Apr 19, 2022

luisenp Apr 19, 2022

luisenp Apr 15, 2022

luisenp Apr 15, 2022

jeffin07 commented Apr 19, 2022

jeffin07 commented Apr 20, 2022 •

edited

Loading

luisenp commented Apr 20, 2022

luisenp Apr 20, 2022

luisenp Apr 20, 2022

luisenp left a comment

jeffin07 commented Apr 20, 2022

jeffin07 commented Apr 21, 2022

luisenp left a comment

luisenp commented Apr 26, 2022

mhmukadam left a comment

jeffin07 commented Apr 27, 2022

		@@ -60,6 +61,30 @@ def test_gp_motion_model_cost_weight_copy():
		)


		def test_gp_motion_model_variable_type():

Allow constant inputs to cost functions to be passed as floats #150

Allow constant inputs to cost functions to be passed as floats #150

Conversation

jeffin07 commented Apr 10, 2022

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

jeffin07 commented Apr 10, 2022

luisenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffin07 commented Apr 13, 2022

luisenp left a comment

Choose a reason for hiding this comment

jeffin07 commented Apr 13, 2022

luisenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffin07 commented Apr 19, 2022

jeffin07 commented Apr 20, 2022 • edited Loading

luisenp commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisenp left a comment

Choose a reason for hiding this comment

jeffin07 commented Apr 20, 2022

jeffin07 commented Apr 21, 2022

luisenp left a comment

Choose a reason for hiding this comment

luisenp commented Apr 26, 2022

mhmukadam left a comment

Choose a reason for hiding this comment

jeffin07 commented Apr 27, 2022

jeffin07 commented Apr 20, 2022 •

edited

Loading