-
Notifications
You must be signed in to change notification settings - Fork 38
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
DOC: Add ellipticity example #471
base: main
Are you sure you want to change the base?
Conversation
Review comments addressed. |
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.
Some general comments:
- Calculating the variance from multiple simulations overcomplicates the code and distracts from the main purpose of the example. I would stick to just a single simulation as in our other examples.
- What is the source of the SDSS data? How was the file generated? Are we licensed to distribute it?
# projected 2D axis ratios. Specifically, it samples the axis ratios of the | ||
# 3D ellipsoid according to the description above [1] and | ||
# then randomly projects them using | ||
# :func:`skypy.utils.random.triaxial_axis_ratio()`. |
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.
Potentially confusing to mention the triaxial_axis_ratio
function. It is hidden from the user who doesn't need to understand the implementation in this context.
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.
Not sure I agree, but I'm open to delete that bit.
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.
To be clear, we still want to explain the model (population of triaxial ellipsoids with some distribution in projection) but the implementation details (ryden04_ellipticity
calls another function triaxial_axis_ratio
) are at best irrelevant and at worst confusing for the user.
lw=0.5, mew=0.5, label='SkyPy model') | ||
|
||
plt.xlabel(r'Axis ratio, $q_{am}$') | ||
plt.ylabel(r'N') |
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.
Units? Do we know the area covered by the data?
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.
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.
yes it's axis ratio and number of galaxies, the area shouldn't come into it
Yes, I agree. However, how could you generate the error bars otherwise? Or you are suggesting we simply draw one sim and plot the histogram? How can we convince the SkyPy model is compatible with SDSS data?
|
No, I queried the data from the SDSS servers because I didn't know. |
Description
This PR includes the validation plot for the 3D ellipticity model (Ryden04) in the examples page.
It reproduces the distribution of axis ratio
𝑞_{am}
for exponential galaxies in the SDSS DR1 as done by @ntessore here.Merging this PR closes #470.
Checklist
References