Implement cartesian_to_origin #28
Conversation
|
@akoumjian @spenczar No rush on this one considering our current higher-priority work. |
moeyensj
force-pushed
the
jm/cartesian-to-origin
branch
from
c6f7a5b
to
fdb8188
Compare
|
|
||
| # Calculate offset in position in mm | ||
| r_diff = np.linalg.norm(diff[:, :3], axis=1) * u.au.to(u.mm) | ||
| # Calculate offset in velocity in nm/s |
| diff = ( | ||
| cartesian_coordinates_barycentric_actual.values | ||
| - cartesian_coordinates_barycentric.values | ||
| ) |
There was a problem hiding this comment.
Feels a bit like we could use a distance (or diff? or offset? or difference?) method on the coordinates table class
There was a problem hiding this comment.
Great idea! Would it be confusing if it returned a residual? (https://github.com/B612-Asteroid-Institute/adam_core/blob/main/adam_core/coordinates/residuals.py#L41)
Maybe we could also define __add__ and such and then check internally if the origins and frames match. If the operators return CartesianCoordinates then properties like .r_mag and .v_mag would give us the position and velocity differences.
There was a problem hiding this comment.
Residuals makes sense to me as a return value.
__add__ kinda makes sense as a sort of "translate" operator, yeah. It's a little trippy to think about that way, but I can see why that's a natural phrasing.
| frame="ecliptic", | ||
| origin=Origin.from_kwargs( | ||
| code=[ | ||
| "SOLAR_SYSTEM_BARYCENTER" |
There was a problem hiding this comment.
| "SOLAR_SYSTEM_BARYCENTER" | |
| OriginCodes.SOLAR_SYSTEM_BARYCENTER |
Does this work? I'd like if it did. Maybe str(OriginCodes.SOLAR_SYSTEM_BARYCENTER) though?
There was a problem hiding this comment.
OriginCodes.SOLAR_SYSTEM_BARYCENTER has integer values corresponding to NAIF IDs. str(OriginCodes.SOLAR_SYSTEM_BARYCENTER) returns "OriginCodes.SOLAR_SYSTEM_BARYCENTER" (which is weird). I think broadly it would be nice if we could drop the strings in favor of storing integers but I'm worried we will lose readability. What do you think?
There was a problem hiding this comment.
Hm, I'm not sure. You could add a __str__ method to OriginCodes that does what you want - returns self.name.
or you could do OriginCodes.SOLAR_SYSTEM_BARYCENTER.name as the literal?
I'm not really convinced these are better in any way. They might be merely different without any actual effect. I think what you have is fine, unless you feel inspired to really dig in.
adam_core/coordinates/transform.py
Outdated
| origins = coords.origin.code.to_numpy(zero_copy_only=False) | ||
| unique_origins = np.unique(origins) |
There was a problem hiding this comment.
Since zero_copy_only=False, it's a good idea to do as much computation as possible in arrow-space:
| origins = coords.origin.code.to_numpy(zero_copy_only=False) | |
| unique_origins = np.unique(origins) | |
| unique_origins = coords.origin.code.unique().to_numpy(zero_copy_only=False) |
There was a problem hiding this comment.
The rewrite is a little more complicated since we use the original non-deduped origin array to create a mask: mask = np.where(origins == origin_in)[0], but I'm inspired. Let me try this fully with pyarrow.
There was a problem hiding this comment.
This e9c6109 implementation still is not zero copy but its copying an array of booleans and not strings so hopefully that is better.
moeyensj
force-pushed
the
jm/cartesian-to-origin
branch
2 times, most recently
from
781a8e9
to
33b886d
Compare
Co-authored-by: Spencer Nelson <[email protected]>
Co-authored-by: Spencer Nelson <[email protected]>
moeyensj
force-pushed
the
jm/cartesian-to-origin
branch
from
33b886d
to
e9c6109
Compare
Also adds barycentric elements to the test data.