-
Notifications
You must be signed in to change notification settings - Fork 9
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
base: master
Are you sure you want to change the base?
Conversation
Can you also update the submodule version? |
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. |
Any updates on this? Note that there are merge conflicts. |
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.
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
- 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")
- 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")
- 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")
- 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
- 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:
Do you know what is the reason for this difference?
<td>Gaussian</td> | ||
</tr> | ||
<tr> | ||
<td>SNIa efficiency per Msun (Gaussian part)</td> |
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.
Wouldn't it be clearer to just give units of 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
.
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, you are right, I will change this
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. |
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.
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" |
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.
All other cases are exponential
?
What if no DTD model is used?
remove f-string Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Change == to is 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]>
This needs merging master into when ready. |
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Co-authored-by: Evgenii Chaikin <[email protected]>
Is this ready for the second review, @Fonotec, or are you planning to make more changes? |
I am making some more edits, I will let you know when I am done. |
@EvgeniiChaikin are you happy with the new version? |
This is an update that is mainly related to SNIa. Below I summarize the changes: