-
Notifications
You must be signed in to change notification settings - Fork 373
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
Nicer transform archetype interface in python #3560
Conversation
…nslationRotationScale3D datatypes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes for such a nice API!!
Two things:
- Maybe add a test to ensure exceptions are raised with incompatible args (
with @pytest.raises(ValueError): ...
. - Maybe should shouldn't completely hide (or make non-obvious) that the "concrete" types (TranslationAndMat3x3, etc.) can be used as well. That could be added to the
__init__
docstring, maybe, in atransform3d_explicit.py
example?
""" | ||
Create a new instance of the Transform3D archetype. | ||
|
||
Parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be an example section here would be good, as a way to list all supported combinations. This is going to be useful for the user, to avoid solving for constraints listed in the Parameter
section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we generally have examples on the type documentation. I'm hesitant to add examples now and here and only for python.
"all supported combintations" may also be way too much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is not the place for a full on example, but something akin to this would still provide value IMO. It's much more explicit as to what the 3 general forms are.
# use one of the concrete transform data type, e.g.
rr.Transform3D(rr.datatypes.TranslationAndMat3x3)
# using translation and/or and 3x3 matrix
rr.Transform3D(translation=[...], mat3x3=[...])
# using any or all of the elementary transform components
rr.Transform3D(translation=[...], rotation=..., scale=...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could even be a bullet list without actual code.
I mean they're not really all that hidden, are they? It's the first parameter the method takes. But that said I could manually expand the type annotation to
but we also never do that in any other place, so I'm a bit hesitant |
Yeah, I don't have a very strong feeling about this. Also, I think my comment would largely address the visibility of these datatypes. |
…ationRotationScale3D` across languages
938bfb3
to
a26c974
Compare
What
TranslationRotationScale3D
andTranslationAndMat3x3
datatypes no longer implement the as_components interfaceChecklist