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

WIP Update the SNIa rate plots in the pipeline #242

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

Fonotec
Copy link
Contributor

@Fonotec Fonotec commented Mar 21, 2023

This is an update that is mainly related to SNIa. Below I summarize the changes:

  1. Update the observational data repository (depends on SWIFTSIM/velociraptor-pipeline-data#115) this MR depends on @MatthieuSchaller
  2. Include where possible the corresponding data in the plots
  3. Plot the logger file for the cosmic SNIa and CC SN rate
  4. Plot the logger file for the cosmic r-processes rates (NS mergers, collapsars, CEJSN) interesting for @correac
  5. Calculate the cosmic SNIa and CC rate directly from the amount of stellar mass formed
  6. I also added a plot with the cosmic ratio of SNIa and CC SN.
  7. Divide the section of SNIa in more sections to keep a good overview. I now have sections for cosmic rates calculated from the stellar mass formed, cosmic rates from loggers, SNIa rates versus gas metallicity, SNIa rates versus SFR or sSFR, SNIa rates versus stellar mass.
  8. I reordered some plots such that the first plots that appear are the plots that we want to see first.
  9. I added a description of the SNIa model to the model description at the top of the webpage.

@MatthieuSchaller
Copy link
Member

Can you also update the submodule version?

@EvgeniiChaikin
Copy link
Collaborator

Thanks @Fonotec!

Just to update you on the status: I have started looking into this PR but it will likely take me a couple of more days to finish the review.

@MatthieuSchaller
Copy link
Member

Any updates on this? Note that there are merge conflicts.

Copy link
Collaborator

@EvgeniiChaikin EvgeniiChaikin left a comment

Choose a reason for hiding this comment

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

Hi @Fonotec !

This branch already looks very good and I really like the variety of new plots and data that it provides!!

I left some small comments below in order to make the scripts more clear.

  • My first major comment is about the structure of the scripts. By comparing your 12 news scripts with one another, I identified 6 pairs of scripts where the differences in the code between the two scripts in the pair are small. Here are these pairs
  1. diff cosmic_SNIa_rate.py cosmic_log_SNIa_rate.py
132a133
> ax.set_yscale("log")
152c153
< ax.set_ylim(0.0, 1.6)
---
> ax.set_ylim(3e-2, 2.0)
162,167c163
< fig.savefig(f"{output_path}/SNIa_rate_history.png")
< 
< ax.set_xlim(1.02, 0.33)
< ax2.set_xlim(1.02, 0.33)
< 
< fig.savefig(f"{output_path}/SNIa_rate_history_zoom.png")
---
> fig.savefig(f"{output_path}/log_SNIa_rate_history.png")
  1. diff cosmic_CC_SN_rate.py cosmic_log_CC_SN_rate.py
131a132
> ax.set_yscale("log")
151c152
< ax.set_ylim(0.0, 26.0)
---
> ax.set_ylim(1e-1, 3e1)
161c162
< fig.savefig(f"{output_path}/CC_SN_rate_history.png")
---
> fig.savefig(f"{output_path}/log_CC_SN_rate_history.png")

  1. diff cosmic_log_NSM_rate.py cosmic_log_collapsar_rate.py
63c63
<     
---
> 
77c77
<         ax.plot(scale_factor, NSM_rate.value * multiplicative_factor, zorder=10000)[0]
---
>         ax.plot(scale_factor, collapsar_rate.value * multiplicative_factor, zorder=10000)[0]
128c128
< ax.set_ylim(1e-5, 1e-1)
---
> ax.set_ylim(1e-5, 2.0)
134c134
<     f"NS-NS merger rate [$10^{{-{log_multiplicative_factor}}}$ yr$^{{-1}}$ cMpc$^{{-3}}$]"
---
>     f"Collapsar rate [$10^{{-{log_multiplicative_factor}}}$ yr$^{{-1}}$ cMpc$^{{-3}}$]"
138c138
< fig.savefig(f"{output_path}/log_NSM_rate_history.png")
---
> fig.savefig(f"{output_path}/log_collapsar_rate_history.png")

  1. diff cosmic_log_NSM_rate.py cosmic_log_CEJSN_rate.py
63c63
<     
---
> 
77c77
<         ax.plot(scale_factor, NSM_rate.value * multiplicative_factor, zorder=10000)[0]
---
>         ax.plot(scale_factor, CEJSN_rate.value * multiplicative_factor, zorder=10000)[0]
134c134
<     f"NS-NS merger rate [$10^{{-{log_multiplicative_factor}}}$ yr$^{{-1}}$ cMpc$^{{-3}}$]"
---
>     f"Common-envelop jets SN rate [$10^{{-{log_multiplicative_factor}}}$ yr$^{{-1}}$ cMpc$^{{-3}}$]"
138c138
< fig.savefig(f"{output_path}/log_NSM_rate_history.png")
---
> fig.savefig(f"{output_path}/log_CEJSN_rate_history.png")

and also
5. diff cosmic_log_SNIa_rate_CSFH_based.py cosmic_SNIa_rate_CSFH_based.py
6. diff cosmic_log_CC_SN_rate_CSFH_based.py cosmic_CC_SN_rate_CSFH_based.py

The pipeline supports an option to use a single script for creating multiple plots (see e.g. https://github.com/SWIFTSIM/pipeline-configs/blob/master/colibre/config.yml#L275). This is done by passing extra parameters to the script to let it know which plot to create. I recommend that you make use of this feature. Then instead of 12 scripts, there will be only 6 scripts, which will thus reduce the amount of code by ~50 per cent (and, hence, the chance of a bug in the code). This will also make the pipeline faster because there will be less repeating calculations and less readings from a file.

  • Second, please adjust the y limits in the CC SN rate plots. My tests of this branch show that sometimes the data can be outside the plots' domains

log_CC_SN_rate_history
CC_SN_rate_history

  • Finally, I have got the impression that the cosmic CC SN rate is higher when computed from the SNII file compared to when computed from the SFH file. Here are the plots of the two rates:

CC_SN_rate_history_based_on_CSFH
CC_SN_rate_history (1)

Do you know what is the reason for this difference?

colibre/scripts/cosmic_SNIa_over_CC_SN_rate_CSFH_based.py Outdated Show resolved Hide resolved
<td>Gaussian</td>
</tr>
<tr>
<td>SNIa efficiency per Msun (Gaussian part)</td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be clearer to just give units of $\rm M_\odot^{-1}$ after the value of this parameter in the right column of the table, instead of saying per Msun in the name of this parameter?

The same comment also applies to the other lines of the code below whose text contains SNIa efficiency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right, I will change this

colibre/description.html Outdated Show resolved Hide resolved
colibre/description.html Outdated Show resolved Hide resolved
colibre/description.html Outdated Show resolved Hide resolved
colibre/auto_plotter/snia_rates.yml Outdated Show resolved Hide resolved
colibre/auto_plotter/snia_rates.yml Outdated Show resolved Hide resolved
colibre/auto_plotter/snia_rates.yml Outdated Show resolved Hide resolved
Comment on lines +49 to +55
title: 'Cosmic SNIa rate'
caption: SNIa rate history plotted directly from the SNIa.txt file produced by SWIFT.
section: SN Rate (cosmic - stochastic)
output_file: SNIa_rate_history.png
- filename: scripts/cosmic_log_SNIa_rate.py
title: 'Cosmic SNIa rate'
caption: SNIa rate history plotted directly from the SNIa.txt file produced by SWIFT.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have plots that show different things (e.g. log vs. linear SNIa rate). But you use the same caption and title for both plots, as if it was the same plot. Please be more specific and reflect in the caption and in the title what is different between the plots. E.g. if the plot is in long, then I would write log both in the caption and in the title.

The same comment applies also to the cosmic CC SN rate. And also SNIa / CC SN both CSFH based and not CSFH based.

elif have_normalization_timescale:
used_DTD = "power-law-beta-one"
else:
used_DTD = "exponential"
Copy link
Collaborator

Choose a reason for hiding this comment

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

All other cases are exponential?

What if no DTD model is used?

Fonotec and others added 8 commits April 28, 2023 09:04
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Change to LaTeX formattting

Co-authored-by: Evgenii Chaikin <[email protected]>
Change to LaTeX formatting

Co-authored-by: Evgenii Chaikin <[email protected]>
Change to LaTeX formatting

Co-authored-by: Evgenii Chaikin <[email protected]>
@MatthieuSchaller
Copy link
Member

This needs merging master into when ready.

@EvgeniiChaikin
Copy link
Collaborator

Is this ready for the second review, @Fonotec, or are you planning to make more changes?

@Fonotec
Copy link
Contributor Author

Fonotec commented May 23, 2023

I am making some more edits, I will let you know when I am done.

@MatthieuSchaller
Copy link
Member

@EvgeniiChaikin are you happy with the new version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants