Implement Inertia object #24588
Implement Inertia object #24588
Conversation
|
✅ Hi, I am the SymPy bot (v169). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.12. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
🟠Hi, I am the SymPy bot (v169). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it. This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75. The following commits add new files:
If these files were added/deleted on purpose, you can ignore this message. |
tjstienstra
force-pushed
the
InertiaObject
branch
from
5053951
to
c61d2ba
Compare
sympy/physics/mechanics/inertia.py
Outdated
| (pos_vec | pos_vec)) | ||
|
|
||
|
|
||
| class Inertia: |
There was a problem hiding this comment.
Should we be subclassing tuple or typing.NamedTuple here in order to achieve correct typing behaviour for backwards compatibility? Inertias were previously tuples, if a user is using an isinstance check then this could fail with Inertia.
There was a problem hiding this comment.
I personally don't like inheriting from tuple, because we'll also get methods which we don't like (such as __add__). I can switch the design of this class to the first example proposed in this comment, which will indeed ensure more complete backwards compatibility.
There was a problem hiding this comment.
I think inheriting from tuple would be the "proper" way to do it. However, I agree that getting tuple's __add__ is a negative so this would need to be overridden. I'd propose subclassing from tuple but then manually overwriting __add__ and __mul__ (and any other unwanted dunder methods) to raise TypeError: + operator is not supported for Inertia or something.
There was a problem hiding this comment.
The other consideration is whether we should subclass from tuple or typing.NamedTuple. See this related comment on another issue.
There was a problem hiding this comment.
The fact that you're considering subclassing tuple makes me wonder why there is a class here at all given that the methods are all just boilerplate. Can you not just use a tuple?
Alternatively if you're considering something like namedtuple then it seems like the modern way is frozen dataclasses.
There was a problem hiding this comment.
Okay, I see why you're doing this. Current code returns a tuple and you want to change it to an object with attributes and methods. In that case namedtuple is specifically designed for this situation. Yes, __add__ might not do what you would ideally want but it will continue to function is it always did for anyone who might happen to be using it already. On the other hand if you actually want to change the behaviour of __add__ to do something more useful then you can still override the __add__ method.
There was a problem hiding this comment.
It is still not very recommended to override namedtuple for builtin methods like __add__, __mul__ to have inconsistent behavior than tuple (such as vector addition), because you need to be careful you don't use that subclass where tuple + tuple is expected to give juxtaposition result, and it is easy to get this bleed through because isinstance(..., tuple) wouldn't guard it.
If we are unhappy with default addition or multiplication of builtin, we would better raise errors early or leave as is, and define other methods that doesn't exist in base class.
There was a problem hiding this comment.
Okay, I've reverted the override, while I have to say that I'm quite sure that most problems were guarded by also overwriting the __radd__ and __rmul__ methods. @sylee957 what did you by the way mean with raise errors early? Was currently raising it directly when calling the + or * operation on an Inertia.
There was a problem hiding this comment.
I don't have strong opinion about whether this shouldn't be subclassed or not, it can be subclassed if it was originally tuple, but only be careful about overriding __add__ and __mul__ to have different behavior, because that specific part is not compatible.
There was a problem hiding this comment.
In that case, I'll revert my last commit and block the usage of __add__ and __mul__, because:
- They'll prevent the misusage of those operations, which is correct from a mechanical point of view.
- In the future it is easier to switch to supporting those operations in a way which is correct from an mechanical point of view. For example
5 * Inertia(dyadic, point) == Inertia(5 * dyadic, point).
P.S. It is of course still possible to write a class that does support these operations with Inertia as in the example below, but this is not the case for int and tuple (also namedtuple), which are also tested. But even if someone were to be in this position, it would otherwise be supported as well, so trying to block it does not result in any damage.
from sympy.physics.mechanics import ReferenceFrame, Inertia, Point
class Opponent:
def __init__(self):
pass
def __add__(self, other):
return self
def __radd__(self, other):
return self
P = Point('P')
N = ReferenceFrame('N')
I = Inertia(P, N.x | N.x)
I + Opponent() # Raises an error
Opponent() + I # Does not raise an error
|
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[41d90958] [24bc2920]
<sympy-1.11.1^0>
- 1.24±0.01ms 811±20μs 0.65 solve.TimeSparseSystem.time_linear_eq_to_matrix(10)
- 3.55±0.07ms 1.53±0.03ms 0.43 solve.TimeSparseSystem.time_linear_eq_to_matrix(20)
- 7.11±0.09ms 2.21±0.06ms 0.31 solve.TimeSparseSystem.time_linear_eq_to_matrix(30)
Full benchmark results can be found as artifacts in GitHub Actions |
tjstienstra
force-pushed
the
InertiaObject
branch
from
85a3ad7
to
41cf7c0
Compare
moorepants
added
the
CZI: Codegen/Biomech
References to other Issues or PRs
This PR is part of the mechanics experimental development (#24234) implementing the changes suggested in #24257.
Brief description of what is fixed or changed
This PR implements an
Inertiaobject that stores the quantity (Dyadic) and the reference (Point) of an inertia. This is a replacement of the tuple, which is currently being used. Besides that the functions associated with inertia are moved to a separate inertia.py file removing some circular import issues.Other comments
Release Notes
NO ENTRY