-
Notifications
You must be signed in to change notification settings - Fork 7
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 from_attitude class method for more permissive init #39
Conversation
ca4dd3d
to
deafcb0
Compare
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.
Looks fine. I added just a few minor comments.
Quaternion/Quaternion.py
Outdated
Attitude(s) as a Quat | ||
""" | ||
if isinstance(att, Quat): | ||
return att |
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.
In all the other cases below, this function returns a new object, while here it returns the same input object. Should we be consistent and return a copy always?
One way this can backfire is when the user creates quaternions within a loop, from a single quaternion instance. All "new" quaternions will actually be a pointer to the same one instance (which will have values from the last iteration).
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.
Good point, will fix.
except Exception: | ||
pass | ||
else: | ||
return att |
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 line was not covered in tests, according to coverage.py
.
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.
Fixed.
Quaternion/Quaternion.py
Outdated
Attitude(s) as a Quat | ||
""" | ||
if isinstance(att, Quat): | ||
return att |
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 line was not covered in tests, according to coverage.py
.
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.
Fixed.
Thanks, I think I addressed all comments. |
Description
This allows more permissive initialization of a Quaternion for cases when the input is known to be an attitude (instead of e.g. a 3x3 transform).
Testing