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

ENH: example for galaxy demographics #520

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Lucia-Fonseca
Copy link
Member

@Lucia-Fonseca Lucia-Fonseca commented Feb 21, 2022

Description

This PR adds an example to reproduce Figure 3 in de la Bella et al. 2021. Include sonification feature from Strauss. Merging this PR closes #514 .
Inputs:

  • Quenching model
  • Weigel et al. 2016 data.

References

  • de la Bella et al. 2021, Quenching and Galaxy Demographics, arXiv 2112.11110.
  • Trayford J., 2021, james-trayford/strauss: v0.1.0 Pre-release, doi:10.5281/zenodo.5776280, https://doi.org/10.5281/ zenodo.5776280

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

@Lucia-Fonseca Lucia-Fonseca self-assigned this Feb 21, 2022
@Lucia-Fonseca Lucia-Fonseca added enhancement Improvement of existing feature examples module: galaxies labels Feb 21, 2022
@Lucia-Fonseca Lucia-Fonseca changed the title Create plot example for galaxy demographics ENH: example for galaxy demographics Feb 21, 2022
@Lucia-Fonseca Lucia-Fonseca marked this pull request as draft February 21, 2022 16:36
@Lucia-Fonseca
Copy link
Member Author

@rrjbca to play the audio files, I'll use this
import IPython IPython.display.Audio("skypy_active.wav")

Does that mean we need to make IPython a dependency in SkyPy only to run the example?

@rrjbca
Copy link
Contributor

rrjbca commented Jun 16, 2022

@rrjbca to play the audio files, I'll use this import IPython IPython.display.Audio("skypy_active.wav")

Does that mean we need to make IPython a dependency in SkyPy only to run the example?

Sorry I only just saw this. Yes you should just add ipython as an extra requirement for docs in setup.cfg. I tested this locally and it worked fine.

@Lucia-Fonseca
Copy link
Member Author

test_smail fails and the checks don't pass

@rrjbca
Copy link
Contributor

rrjbca commented Jun 17, 2022

test_smail fails

Check the logs, it's a kstest failure, just re-run it

@Lucia-Fonseca
Copy link
Member Author

The docs are not picking up the latest changes but it renders well on my local machine.
This needs #516 to be merged before this is ready.

Copy link
Contributor

@philipp128 philipp128 left a comment

Choose a reason for hiding this comment

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

Thanks @Lucia-Fonseca, the example looks really nice. Please find some comments below.

One thing beside the comments is that it might be good to increase the tick labels. They seem a bit small.


for ax in [ax1, ax2, ax3]:
ax.legend(loc='lower left', fontsize='small', frameon=False)
ax.set_xlabel(r'Stellar mass, $log (M/M_{\odot})$', fontsize=18)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ax.set_xlabel(r'Stellar mass, $log (M/M_{\odot})$', fontsize=18)
ax.set_xlabel(r'Stellar mass, $\log (M/M_{\odot})$', fontsize=18)

This is to prevent log from being in italics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe increase the fontsize for both labels

ax.set_yscale('log')


ax1.set_ylabel(r'$\phi /h^3 Mpc^{-3}$', fontsize=18)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ax1.set_ylabel(r'$\phi /h^3 Mpc^{-3}$', fontsize=18)
ax1.set_ylabel(r'$\phi /h^3$\,Mpc$^{-3}$', fontsize=18)

I prefer and learned to not put the units into math mode. This makes it easier to distinguish between parameters/variables and units.

Comment on lines +108 to +111
z_centrals, m_centrals = schechter_smf(z_range, mstarb, phic, alphab, 1e9, 1e12, sky_area, cosmology)
z_satellites, m_satellites = schechter_smf(z_range, mstarb, phis, alphab, 1e9, 1e12, sky_area, cosmology)
z_massq, m_mass_quenched = schechter_smf(z_range, mstarb, phimq, alphab + 1, 1e9, 1e12, sky_area, cosmology)
z_satq, m_satellite_quenched = schechter_smf(z_range, mstarb, phisq, alphab, 1e9, 1e12, sky_area, cosmology)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
z_centrals, m_centrals = schechter_smf(z_range, mstarb, phic, alphab, 1e9, 1e12, sky_area, cosmology)
z_satellites, m_satellites = schechter_smf(z_range, mstarb, phis, alphab, 1e9, 1e12, sky_area, cosmology)
z_massq, m_mass_quenched = schechter_smf(z_range, mstarb, phimq, alphab + 1, 1e9, 1e12, sky_area, cosmology)
z_satq, m_satellite_quenched = schechter_smf(z_range, mstarb, phisq, alphab, 1e9, 1e12, sky_area, cosmology)
z_centrals, m_centrals = schechter_smf(z_range, mstarb, phic, alphab,
1e9, 1e12, sky_area, cosmology)
z_satellites, m_satellites = schechter_smf(z_range, mstarb, phis, alphab,
1e9, 1e12, sky_area, cosmology)
z_massq, m_mass_quenched = schechter_smf(z_range, mstarb, phimq, alphab + 1,
1e9, 1e12, sky_area, cosmology)
z_satq, m_satellite_quenched = schechter_smf(z_range, mstarb, phisq, alphab,
1e9, 1e12, sky_area, cosmology)

Adding line breaks prevents the user from scrolling horizontally in the docs.

Comment on lines +44 to +48
# from skypy.galaxies.stellar_mass import (schechter_smf_amplitude_centrals,
# schechter_smf_amplitude_satellites,
# schechter_smf_amplitude_mass_quenched,
# schechter_smf_amplitude_satellite_quenched
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this can only be included after #516 is merged. Please adapt the indentation when when this PR is ready to be merged.

# Schechter Parameters
# --------------------
#
# We generate their Figure 4 and use the best-fit values of the blue parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# We generate their Figure 4 and use the best-fit values of the blue parameters
# We generate Figure 3 in de la Bella et al. 2021 and use the best-fit values of the blue parameters

Comment on lines +166 to +185
fig, (ax1, ax2, ax3) = plt.subplots(nrows=1, ncols=3, figsize=(16, 6), sharex=True, sharey=True)
fig.suptitle('Galaxy Demographics Simulation', fontsize=26)

ax1.plot(wblue['logm'], 10**wblue['logphi'], color='k', label='Weigel+16 active', lw=1)
ax1.step(bins[:-1], logphi_active, where='post', label='SkyPy active', color='blue', zorder=3, lw=1)
ax1.step(bins[:-1], logphi_centrals, where='post', label='SkyPy centrals', color='royalblue', zorder=3, lw=1)
ax1.step(bins[:-1], logphi_satellites, where='post', label='SkyPy satellites', color='cyan', zorder=3, lw=1)


ax2.plot(wred['logm'], 10**wred['logphi'], color='k', label='Weigel+16 passive', lw=1)
ax2.fill_between(wred['logm'], 10**wred['upper_error'], 10**wred['lower_error'], color='salmon', alpha=0.1)
ax2.step(bins[:-1], logphi_passive, where='post', label='SkyPy passive', color='red', zorder=3, lw=1)
ax2.step(bins[:-1], logphi_massq, where='post', label='SkyPy mass-quenched', color='coral', zorder=3, lw=1)
ax2.step(bins[:-1], logphi_satq, where='post', label='SkyPy sat-quenched', color='maroon', zorder=3, lw=1)

ax3.plot(wtotal['logm'], 10**wtotal['logphi'], color='k', label='Weigel+16 total', lw=1)
ax3.plot(wcentral['logm'], 10**wcentral['logphi'], '--', color='grey', label='Weigel+16 centrals', lw=1)
ax3.plot(wsatellite['logm'], 10**wsatellite['logphi'], '--', color='grey', label='Weigel+16 satellites', lw=1)
ax3.fill_between(wtotal['logm'], 10**wtotal['upper_error'], 10**wtotal['lower_error'], color='plum', alpha=0.1)
ax3.step(bins[:-1], logphi_total, where='post', label='SkyPy total', color='purple', zorder=3, lw=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add line breaks here such that the user does not need to scroll horizontally?

Comment on lines +182 to +183
ax3.plot(wcentral['logm'], 10**wcentral['logphi'], '--', color='grey', label='Weigel+16 centrals', lw=1)
ax3.plot(wsatellite['logm'], 10**wsatellite['logphi'], '--', color='grey', label='Weigel+16 satellites', lw=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to plot these two lines with the same line style?

Copy link
Contributor

Choose a reason for hiding this comment

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

What are these lines supposed to demonstrate in the first place? Does it make sense to remove them or also show the SkyPy fits of these two components?

fig, (ax1, ax2, ax3) = plt.subplots(nrows=1, ncols=3, figsize=(16, 6), sharex=True, sharey=True)
fig.suptitle('Galaxy Demographics Simulation', fontsize=26)

ax1.plot(wblue['logm'], 10**wblue['logphi'], color='k', label='Weigel+16 active', lw=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

The Weigel+ lines for the passive and the total sample include errors. Does the blue sample not have errors?



ax1.set_ylabel(r'$\phi /h^3 Mpc^{-3}$', fontsize=18)
# plt.savefig('galaxy_simulation.pdf')
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this comment

# is becoming increasingly important to make astronomy accessible
# for those who are visually impaired, and to enhance visualisations
# and convey information that visualisation alone cannot.
# In this work [1] the authors also made their main plot available
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# In this work [1] the authors also made their main plot available
# In [1]_, the authors also made their main plot available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing feature examples module: galaxies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Quenching example
4 participants