Skip to content
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

Make Body and ReferenceFrame analogous #21964

Closed
sidhu1012 opened this issue Aug 28, 2021 · 18 comments
Closed

Make Body and ReferenceFrame analogous #21964

sidhu1012 opened this issue Aug 28, 2021 · 18 comments
Labels
Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. physics.mechanics physics.vector physics

Comments

@sidhu1012
Copy link
Contributor

sidhu1012 commented Aug 28, 2021

Body and ReferenceFrame are analogous in dynamics, and all the joint classes , jointsmethod and class body follow it, but rest of physics module doesn't.

Example -

>>> A = Body('A')
>>> N = ReferenceFrame('N')
>>> A.dcm(N) # Works
>>> A.dcm(A) # Works

>>> N.dcm('A') # Error but should work

So where ever ReferenceFrame works on being passed as arg, passing body to same arg should also work.

It can be done simply by -

if isinstance(frame, Body):
    frame = Body.frame

cc - @moorepants

@sidhu1012 sidhu1012 added Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. physics.mechanics physics physics.vector labels Aug 28, 2021
@mohajain
Copy link
Contributor

I would like to work on this.

It can be done simply by -

if isinstance(frame, Body):
    frame = Body.frame

Where do I have to add this line?

@sidhu1012
Copy link
Contributor Author

Where do I have to add this line?

In physics module wherever we can pass ReferenceFrame, we should be able to pass Body.

@mohajain
Copy link
Contributor

The physics module is really vast, I'm unable to find such functions. Please guide me through, I'm new to sympy.

@sidhu1012
Copy link
Contributor Author

The one's that I remember are Physics.mechanics.functions module(Inertia and all) , physics.mechanics.vector.frame (orient functions) and many more.

All of the functions would be in mechanics and vector sub module.

@mohajain
Copy link
Contributor

Thanks a lot!
Do I have to change these lines

if not isinstance(frame, ReferenceFrame):
        raise TypeError('Please enter a valid ReferenceFrame')

to something like this

if not isinstance(frame, ReferenceFrame) and not isinstance(frame, Body):
        raise TypeError('Please enter a valid ReferenceFrame')

as well?

@sidhu1012
Copy link
Contributor Author

Only change required is to add seperate check if frame's a body before checking frame.

@moorepants
Copy link
Member

It would be nice if duck typing worked, i.e. add all methods to body that mimic ReferenceFrame.

@sidhu1012
Copy link
Contributor Author

It would be nice if duck typing worked, i.e. add all methods to body that mimic ReferenceFrame.

Yes that's the end goal, Body should have everything possible that Particle, Rigid body and Reference Frame has.

@yasphy
Copy link

yasphy commented Nov 2, 2021

Is this issue still open?

@noob8boi
Copy link

noob8boi commented Nov 5, 2021

I would like to work on this issue. I am pretty new to open source development so I think this problem will be a great way to start

@tjstienstra
Copy link
Contributor

The current solutions here seem to focus on checking on every usage whether something is not a Body. Wouldn't it be better to instead change the Body class such that it responds as a ReferenceFrame?
I am currently experimenting with something like:

def __getattr__(self, name):
    if hasattr(self.frame, name):
        return self.frame.__getattribute__(name)
    else:
        raise AttributeError

This is why I use __getattr__. This part will solve the direct calls on the Body, when an attribute is needed like body.x. But it will give error on the isinstance checks. I have not yet figured out how to nicely fake a type, also not sure if we should even want it.

@tjstienstra
Copy link
Contributor

tjstienstra commented Jul 4, 2022

I may have a better solution, since the solution I just proposed, will probably be too tricky to get working nicely. It is also a bit more like the current proposed implementation with isinstance, however it would for example also work for RigidBody.
The solution I propose can have two forms:

  • Add a method get_frame to the classes, which should function as a ReferenceFrame (i.e. ReferenceFrame, Body, RigidBody). And instead of the isinstance we use frame = frame.get_frame().
  • Replace the _check_frame calls for _get_frame (shown below)
def _get_checked_frame(other):
    frame = getattr(other, 'frame', other)
    _check_frame(frame)
    return frame

The advantage of these solutions is that it will be more future proof and it will also return the responsibility a bit more to the classes themselves, instead of to the client.

@sujantkumarkv
Copy link

Hey there!
I request a response on the status of this issue.

@tjstienstra @sidhu1012 @mohajain @yasphy @Infinil

@tjstienstra
Copy link
Contributor

I cannot seem to find a discussion I had about whether it should entirely stay composition or not. I thought that the conclusion of that discussion was that it would be better to just leave it as composition and let them not become entirely analogous. However I cannot entirely confirm this, as I cannot find where that discussion took place.

Another argument to maybe close this issue is that the Body class might get deprecated for several reasons (as mentioned in #24256). The main argument is that it is more reasonable to that Particle and RigidBody inherit from a BodyBase, instead that Body inherits them both. This is more sensible from a physics point of view, but it is also nicer in programming, due to possible problems caused by multiple inheritance. Besides that it is actually possible to support Particle in the joints framework, so there is no real need for Body.

@sujantkumarkv
Copy link

sujantkumarkv commented Mar 11, 2023

Hey!
As far as I read (and please correct me): BodyBase has been implemented but Body hasn't been deprecated according to pull #24234.

So, as per this issue, we started off with making Body and ReferenceFrame analogous but currently stand on leaving it as composition and because Body would be deprecated sooner or later.
I came via GSOC Ideas'23 from Bug burn prioritizing to solve & close left issues.

So, with all said above, doesn't it fit that this issue can be closed now or after deprecating Body? and that we don't need further work on this? Please let me know or assign me if further work is required to close this.
Thanks.

@sujantkumarkv
Copy link

Body and ReferenceFrame are analogous in dynamics, and all the joint classes , jointsmethod and class body follow it, but rest of physics module doesn't.

Example -

>>> A = Body('A')
>>> N = ReferenceFrame('N')
>>> A.dcm(N) # Works
>>> A.dcm(A) # Works

>>> N.dcm('A') # Error but should work

So where ever ReferenceFrame works on being passed as arg, passing body to same arg should also work.

It can be done simply by -

if isinstance(frame, Body):
    frame = Body.frame

cc - @moorepants

I would also like to mention regarding the original issue:
A.dcm(N) doesn't work & neither does N.dcm(A), A.dcm(A) returns :: Matrix([[1, 0, 0], [0, 1, 0], [0, 0, 1]])

@sujantkumarkv
Copy link

Hey @tjstienstra,

Based on this comment of yours, I would think that it's best to close this issue as to keep things in order and clean.
Thanks.

@shishir-11
Copy link
Contributor

I think this issue needs to be closed since Body has been deprecated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. physics.mechanics physics.vector physics
Projects
None yet
8 participants