-
Notifications
You must be signed in to change notification settings - Fork 100
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
minor astropy units + mass calc fixes #577
Conversation
Thanks! Looks like there's a typo in the code that needs to be fixed. Otherwise, two more comments:
|
Let me double check but I'm positive I uncommented the try-catch statement to fix something (one of the power law potentials I think) that was failing because it was passing into the numerical integration in |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #577 +/- ##
========================================
Coverage 99.91% 99.91%
========================================
Files 196 198 +2
Lines 28955 29184 +229
========================================
+ Hits 28931 29160 +229
Misses 24 24
☔ View full report in Codecov by Sentry. |
Okay I confirmed that vectorized mass calculations fails for I could edit this PR then to go back to simple mass calculation without the try-catch sequence, and then work on the list comprehension in a different PR? Would this then still require a test if that PR would be accompanied by tests for vectorized mass calculations using Reproducing mass failure for PowerSphericalPotential() w/ vector:
|
I think we should just use the |
Thanks! |
rmin
as astropy quantity tosphericaldf.sample()
always fails the comparison withself._rmin_sampling
, so parse to internal units first._make_cmf_interpolator()
to allow galpy potentials that don't have vectorized mass calculations to work properly.use_physical=False
to the mass normalization forself._rmin_sampling > 0
so it works properly when denspot has ro,vo set.