Changed classes to inherete from compas.Data #387

yck011522 · 2023-10-04T11:47:09Z

Some of the classes in compas_fab is still based on (object) instead of (Data)

I have selected some of the classes that should support serialization and upgrade them. Upgrading them should be backward compatible. However, it may affect user classes inherited from these classes. This RP does not cover all the classes yet (e.g. classes in backends, reachability map, ghpython and some more). Let me know if you think I should extend to cover all classes.

@gonzalocasas Should I add test files for this?

What type of change is this?

Bug fix in a backwards-compatible manner.
New feature in a backwards-compatible manner.
Breaking change: bug fix or new feature that involve incompatible API changes.
Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
I ran all tests on my computer and it's all green (i.e. invoke test).
I ran lint on my computer and there are no errors (i.e. invoke lint).
I added new functions/classes and made them available on a second-level import, e.g. compas_fab.robots.CollisionMesh.
I have added tests that prove my fix is effective or that my feature works.
I have added necessary documentation (if appropriate)

gonzalocasas

Classes that inherit from Data should also add implementations for the data property getter and setter, and I think some of these ones dont. At least RobotSemantics. Also, the Data class implements a default from_data class method that works only if the class has a constructor with all optional params, if that's not the case, there needs to be an overload of from_data in the class that is able to create instances (usually by first getting the args needed from the data dictionary, constructing with the minimum, and then calling the .data = setter.

gonzalocasas · 2023-10-04T13:03:14Z

Should I add test files for this?

yes please! they don't need to be super elaborate, but testing the serialization and deserialization on all these classes with some basic data would help ensure they behave properly all the time

yck011522 · 2023-10-05T06:27:38Z

I have revised the list of affected classes:

CollisionMesh (using existing to_data() from_data() )
AttachedCollisionMesh (using existing to_data() from_data() )
Robot (using existing data getter setter)
RobotSemantics (added new data getter setter and to_data())
Tool (using existing data getter setter)

The following class is skipped as it maybe redesigned in near future

PlanningScene

yck011522 · 2023-10-31T12:24:01Z

ping @gonzalocasas

chenkasirer

LGTM. left some comments though.

chenkasirer · 2023-11-03T09:30:11Z

src/compas_fab/robots/robot.py

+        self._scale_factor = data.get("_scale_factor", 1.0)
+        self._attached_tools = data.get("_attached_tools", {})
+        self._current_ik = data.get("_current_ik", {"request_id": None, "solutions": None})


mmh is this to stay compatible with previously serialized Robots? But it didn't implement the Data interface before, so there shouldn't be any, right?

You could also simply omit these from the Data dictionary if they take default values, __init__ is ran first anyways. Otherwise you can assume they're there, because they're put there by the getter.

chenkasirer · 2023-11-03T09:30:21Z

src/compas_fab/robots/robot.py

+            "_scale_factor": self._scale_factor,
+            "_attached_tools": self._attached_tools,
+            "_current_ik": self._current_ik,


why the _s?

I didn't invent this particular variable, and it was there all along, I just serialized it. I believe the intent of the existing code is that the model is not supposed to keep track of extrinsic or modifiable properties of the robot (in this case, the current_ik ). Hence the private _ notation. I should probably have another PR to address the intrinsic vs extrinsic state of the robot problem.

I would remove the _ in the keys.

also, current_ik should not be serialized

chenkasirer · 2023-11-03T09:41:22Z

src/compas_fab/robots/semantics.py

+    @classmethod
+    def from_data(cls, data):
+        robot_semantics = cls(None)
+        robot_semantics.data = data
+        return robot_semantics


@gonzalocasas commented on this. I'd say setting robot_model=None in arguments of __init__ is legit. Then you don't need a custom from_data. While I was not a fan, this convention grew on me.

Having said that, the trend in COMPAS 2.0 is to get rid of the data.setter acknowledging it's often redundant. So you'd then just have a data getter and a from_data which feeds the data dict to __init__.

Example (just for illustration, not sure if this works in in LTS): gramaziokohler/compas_timber@d11d83e#diff-5cbdc4545536c895404b80de4fdcc522ec5b95dfb4af73a47e4120888cc22031R42-R48

I'm not sure if I get your argument.

Are you saying that the style is moving towards having default values for all parameters in the __init__ and then skipping the def from_data()?

Probably it would be good to have a more wholistic stylistic clean up separately. The style currently used in compas_fab is a mixed bag of things. I can do that as long as we can have some consensus on what convention we adopt.

yea sorry didn't wanna cause confusion. Let's leave it like that.

tests/robots/test_robot.py

chenkasirer · 2023-11-03T09:47:43Z

tests/robots/test_robot.py

+    assert tool1.name == tool2.name
+    assert str(tool1.frame) == str(tool2.frame)
+
+


I'd add another unittest checking deepcopy. should behave the same as (de)serializing but sometimes doesn't

I tried deepcopy but It doesn't seem to work. I think some years ago, I heard from someone to not use deepcopy on these objects. I don't remember why. But basically I never deepcopied any compas objects since.

Just to give you a taste of that error message:

./tests/robots/test_robot.py::test_robot_deepcopy_with_tool Failed: [undefined]AttributeError: 'Visual' object has no attribute '_guid' tests\robots\test_robot.py:345: in test_robot_deepcopy_with_tool robot2 = deepcopy(robot) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:172: in deepcopy y = _reconstruct(x, memo, *rv) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:271: in _reconstruct state = deepcopy(state, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:172: in deepcopy y = _reconstruct(x, memo, *rv) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:271: in _reconstruct state = deepcopy(state, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:172: in deepcopy y = _reconstruct(x, memo, *rv) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:271: in _reconstruct state = deepcopy(state, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:206: in _deepcopy_list append(deepcopy(a, memo)) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:172: in deepcopy y = _reconstruct(x, memo, *rv) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:271: in _reconstruct state = deepcopy(state, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy y = copier(x, memo) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:206: in _deepcopy_list append(deepcopy(a, memo)) ..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:161: in deepcopy rv = reductor(4) ..\..\..\anaconda3\envs\cf-dev\Lib\site-packages\compas\data\data.py:121: in __getstate__ "guid": str(self.guid), ..\..\..\anaconda3\envs\cf-dev\Lib\site-packages\compas\data\data.py:200: in guid if not self._guid: E AttributeError: 'Visual' object has no attribute '_guid'

it's known deepcopy errors produce the most beautiful stacks..
To this specific one, could there be a class called Visual (really?) somewhere which inherits from Data but doesn't call super().__init__()?

https://github.com/compas-dev/compas/blob/ba932fcb55b2839ef493f4181b1f080f377a772b/src/compas/robots/model/link.py#L166-L191

It looks okay here. I'm not sure how this Visual object is created. It seems that there is a Geometry class within it and that is created during from_data():

https://github.com/compas-dev/compas/blob/ba932fcb55b2839ef493f4181b1f080f377a772b/src/compas/robots/model/link.py#L244-L248

If someone wants to debug this, this is the test code:

def test_robot_deepcopy(panda_robot_instance): import copy robot = panda_robot_instance robot.scale(0.001) robot2 = copy.deepcopy(robot) assert robot is not robot2 def test_robot_deepcopy2(ur5_robot_instance, robot_tool1): import copy robot = ur5_robot_instance robot.attach_tool(robot_tool1) robot2 = copy.deepcopy(robot) assert robot is not robot2

Both test won't work and they have different error message

yck011522 · 2023-11-06T02:36:21Z

ping @gonzalocasas

gonzalocasas

Generally LGTM, I added some feedback though

gonzalocasas · 2023-11-07T09:03:38Z

src/compas_fab/robots/robot.py

+            "_scale_factor": self._scale_factor,
+            "_attached_tools": self._attached_tools,
+            "_current_ik": self._current_ik,


I would remove the _ in the keys.

gonzalocasas · 2023-11-07T09:05:43Z

src/compas_fab/robots/robot.py

+            "_scale_factor": self._scale_factor,
+            "_attached_tools": self._attached_tools,
+            "_current_ik": self._current_ik,


also, current_ik should not be serialized

src/compas_fab/robots/semantics.py

tests/robots/test_robot.py

src/compas_fab/robots/robot.py

Co-authored-by: Gonzalo Casas <[email protected]>

…compas_fab into migration_data_class

gonzalocasas · 2023-11-07T13:29:53Z

Cool, thanks for the updates! On my side, I think this is ready to go!

yck011522 added 2 commits October 4, 2023 19:09

Changed classes to inherete from compas.Data

6117719

Fix Lint Problems

bf71c1f

gonzalocasas reviewed Oct 4, 2023

View reviewed changes

yck011522 added 2 commits October 5, 2023 14:10

undo PlanningScene(Data)

3dc9359

Data setter getter for RobotSemantics

d9e15b0

yck011522 added 4 commits October 5, 2023 14:33

changelog

03c6573

Serialization of disabled_collisions in RobotSemantics

4889328

Tests for RobotSemantics and Robot serialization

b2ab248

Avoid non str comparision in tests

4c5aa14

yck011522 marked this pull request as ready for review October 5, 2023 08:59

yck011522 requested review from chenkasirer and gonzalocasas October 5, 2023 09:00

chenkasirer approved these changes Nov 3, 2023

View reviewed changes

lint

23376ab

yck011522 mentioned this pull request Nov 6, 2023

Make sure all entities inherit from Data #358

Open

gonzalocasas reviewed Nov 7, 2023

View reviewed changes

yck011522 and others added 4 commits November 7, 2023 21:08

removed the underscore keys of the serialized dictionary

da851d9

group name assertion

0883f07

simplify comment line

28c814b

Co-authored-by: Gonzalo Casas <[email protected]>

Merge branch 'migration_data_class' of https://github.com/compas-dev/…

c44bb7f

…compas_fab into migration_data_class

yck011522 requested a review from gonzalocasas November 7, 2023 19:30

yck011522 merged commit 88f3551 into main Nov 8, 2023
8 checks passed

yck011522 deleted the migration_data_class branch November 8, 2023 04:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed classes to inherete from compas.Data #387

Changed classes to inherete from compas.Data #387

yck011522 commented Oct 4, 2023 •

edited

Loading

gonzalocasas left a comment

gonzalocasas commented Oct 4, 2023

yck011522 commented Oct 5, 2023

yck011522 commented Oct 31, 2023

chenkasirer left a comment

chenkasirer Nov 3, 2023

chenkasirer Nov 3, 2023

yck011522 Nov 4, 2023

gonzalocasas Nov 7, 2023

gonzalocasas Nov 7, 2023

chenkasirer Nov 3, 2023

yck011522 Nov 4, 2023

chenkasirer Nov 7, 2023

chenkasirer Nov 3, 2023

yck011522 Nov 6, 2023

chenkasirer Nov 7, 2023

yck011522 Nov 7, 2023

yck011522 Nov 7, 2023

yck011522 commented Nov 6, 2023

gonzalocasas left a comment

gonzalocasas Nov 7, 2023

gonzalocasas Nov 7, 2023

gonzalocasas commented Nov 7, 2023

		assert tool1.name == tool2.name
		assert str(tool1.frame) == str(tool2.frame)

Changed classes to inherete from compas.Data #387

Changed classes to inherete from compas.Data #387

Conversation

yck011522 commented Oct 4, 2023 • edited Loading

What type of change is this?

Checklist

gonzalocasas left a comment

Choose a reason for hiding this comment

gonzalocasas commented Oct 4, 2023

yck011522 commented Oct 5, 2023

yck011522 commented Oct 31, 2023

chenkasirer 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

yck011522 commented Nov 6, 2023

gonzalocasas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonzalocasas commented Nov 7, 2023

yck011522 commented Oct 4, 2023 •

edited

Loading