-
Notifications
You must be signed in to change notification settings - Fork 8
Add Zhao potential model #761
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
base: main
Are you sure you want to change the base?
Conversation
|
From @jnibauer:
I haven't run any benchmarks yet, but step 1 is to get a working model in and then we can see if there are any optimizations we can make! |
|
Also I didn't export this to the public API in case we get #757 in. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #761 +/- ##
==========================================
+ Coverage 95.84% 95.90% +0.06%
==========================================
Files 158 159 +1
Lines 5963 6053 +90
==========================================
+ Hits 5715 5805 +90
Misses 248 248 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ss settings of power-law indices)
|
The latest commits make a conceptual change to the potential model after I realized there was a slight bug in the previous math. The mass parameter can't be total mass because models with beta <= 3 do not have finite mass -- that's fine and was working before. But: The density normalization (rho0 in my notation, C in Zhao's notation) was failing for beta<=3, causing the model to return nan for those cases. After a bit of a rabbit hole, long story short, this is because |
jnibauer
left a comment
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.
New mass definition looks good to me! I like this way of setting it more anyways.
| beta = eqx.error_if(beta, beta <= 3.0, "Beta must be >3 to have finite mass.") | ||
| usys = u.unitsystem(units) | ||
| params = { | ||
| "r_s": u.ustrip(usys["length"], r_s) if hasattr(r_s, "unit") else r_s, |
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.
This can use u.ustrip(AllowValue
|
@adrn we should make a benchmark suite. |


This adds the (alpha,beta,gamma) Zhao (1996) double power law model. This replaces #755. Fixes #726.
The tests are currently failing but will pass once #760 is merged.