sot / Quaternion

Quaternion manipulation
https://sot.github.io/Quaternion
BSD 3-Clause "New" or "Revised" License
3 stars 7 forks source link

Define new API for Quat #9

Closed jeanconn closed 4 years ago

jeanconn commented 5 years ago

Define new initializer/API for Quat

@taldcroft Do you want to update this issue with your idea of how you'd like the new initializer to look? I got the impression that you had in mind:

Quat([a, b, c, d]), Quat([ra,dec,roll]) would still work but Quat(q=(n,m,4)) would now use the last dim as the 4 vector to initialize an n by m array of quaternion objects Quat(T=(n,m,z,3,3)) would use the last 2 dims as the 3x3 array to initialize an n by m by z array of quaternion objects

javierggt commented 5 years ago

What I understood was that the constructor would have the attitude parameter set to None by default. Then there would be three other parameters (also None by default): transform, quaternion, equatorial.

Then, when the constructor is called, one and only one of them can be not None, and that determines the behavior:

javierggt commented 5 years ago

And the other option I thought you might have meant is: the constructor would have a required attitude parameter. Then there would be three other parameters (False by default): transform, quaternion, equatorial.

Then, when the constructor is called:

In both cases, the get methods will squeeze the return value, and the set method will store them in vector form (adding a dimension if necessary). The code in the branch already does that.

taldcroft commented 5 years ago

@javierggt has got it mostly right for what I had proposed, where ... can be empty so you could do (and would be encouraged to do):

q = Quat(q=[0, 0, 0, 1])
q = Quat(equatorial=[10, 20, 30])
q = Quat(equatorial=[[10, 20, 30], [0, 1, 2]])

In other words, with the new API we encourage using the keyword args in all cases to make the code more clear. This is part of improving code by making types a bit more explicit. But the keyword args are required for vectorized inputs.

The only change from what is written is that the existing attribute for the 4-element quaternion is q not quaternion, so the keyword arg should match that attribute name.

jeanconn commented 5 years ago

And I think we are all agreeing; I just hadn't figured out what logic you would need to deal with having either an argument or keyword args or both.

jeanconn commented 4 years ago

I think this is probably superseded by #11

javierggt commented 4 years ago

I think this can be closed. This API was already implemented in PR #5. True, the documentation is mostly left for #11.