Skip to content
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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Lucia-Fonseca
Copy link
Member

@Lucia-Fonseca Lucia-Fonseca commented Jul 13, 2021

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

  • Follow the Contributor Guidelines
  • Write unit tests
  • Write documentation strings
  • Assign someone from your working team to review this pull request
  • Assign someone from the infrastructure team to review this pull request

References

@Lucia-Fonseca Lucia-Fonseca added documentation Improvements or additions to documentation module: galaxies examples labels Jul 13, 2021
@Lucia-Fonseca Lucia-Fonseca self-assigned this Jul 13, 2021
@Lucia-Fonseca Lucia-Fonseca marked this pull request as draft July 13, 2021 11:27
@Lucia-Fonseca Lucia-Fonseca changed the title DOC: Add placeholder for ellipticity example DOC: Add ellipticity example Jul 22, 2021
@Lucia-Fonseca
Copy link
Member Author

Lucia-Fonseca commented Jul 22, 2021

To clarify:

  • What does Ryden04 return exactly, the total ellipticity? See issue #475

@Lucia-Fonseca Lucia-Fonseca marked this pull request as ready for review July 22, 2021 11:30
@Lucia-Fonseca Lucia-Fonseca requested review from ntessore and a team July 22, 2021 11:30
@Lucia-Fonseca
Copy link
Member Author

Review comments addressed.

Copy link
Contributor

@rrjbca rrjbca left a 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?

examples/galaxies/plot_ellipticity.py Outdated Show resolved Hide resolved
# 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()`.
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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')
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say these quantities are dimensionless, right? One is a ratio and the other one is simply a count.
@ntessore ANy idea about the area? According to their paper, DR1 is 2099 deg2.

Copy link
Member

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

examples/galaxies/plot_ellipticity.py Outdated Show resolved Hide resolved
examples/galaxies/plot_ellipticity.py Outdated Show resolved Hide resolved
@Lucia-Fonseca
Copy link
Member Author

Lucia-Fonseca commented Sep 6, 2021

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.

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?

  • What is the source of the SDSS data? How was the file generated? Are we licensed to distribute it?
    @ntessore since I took this from your reproduce page. Do you have any idea?

@ntessore
Copy link
Member

ntessore commented Sep 6, 2021

@ntessore since I took this from your reproduce page. Do you have any idea?

No, I queried the data from the SDSS servers because I didn't know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation examples module: galaxies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: Validation 3D ellipticity distribution
4 participants