-
Notifications
You must be signed in to change notification settings - Fork 3
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 support for dynamical simulation unit systems (G=1 or other value) #225
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
==========================================
+ Coverage 96.03% 96.11% +0.07%
==========================================
Files 41 41
Lines 1565 1596 +31
==========================================
+ Hits 1503 1534 +31
Misses 62 62 ☔ View full report in Codecov by Sentry. |
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.
It all looks good! My only thing is it feels like we can shorten the names.
This is to add support for unit systems where only two of three of
{length, mass, time}
units need to be specified because the value of G is set to 1 (or some other value). This is analogous to Gala'sSimulationUnitSystem
.For the value of G, I was originally planning to accept
unxt.Quantity | float | int
. But I think this needs to be a concrete value as the computed units need to be passed tounxt.units()
, which under the hood callsastropy.units.Unit
. So the current approach acceptsastropy.units.Quantity | float | int
, but now I'm thinking that this should instead require onlyfloat | int
because it doesn't really make sense to allow someone to pass in a unit-ful G. That would also simplify some logic here. Thoughts?