Dveyrat/extended cooling tail model #312
Conversation
|
Before completing the review, let's close the loop with collaborators from Hyper-K on this feature (email forwarded by @JostMigenda on 5 Jun 2024). |
* remove unused imports * give __init__ argument a cleaner name * move type check to top of __init__
|
Had a first look while waiting for my transfer in ORD. Boarding begins now, so just a few notes for next week:
|
doc/nb/ccsn/Nakazato_2013.ipynb
Outdated
There was a problem hiding this comment.
It looks like this is just an unintended metadata change; I could try to rebase the branch and make this change disappear, to clean up the PR a little bit.
| def get_integrated_luminosity(self, m_PNS, SRT, alpha, beta, gamma): | ||
| """Get integrated neutrino luminosity from shock revival time to 20 s. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| m_PNS : astropy.Quantity | ||
| Mass of the proto-neutron star. | ||
| SRT : astropy.Quantity | ||
| Shock revival time. | ||
| alpha : float | ||
| First cooling tail model parameter. | ||
| beta : float | ||
| Second cooling tail model parameter. | ||
| gamma : float | ||
| Third cooling tail model parameter. | ||
|
|
||
| Returns | ||
| ------- | ||
| astropy.Quantity | ||
| Integrated luminosity calculated from Ekanger et al. (2023) model. | ||
| """ | ||
| logE = (m_PNS.value * alpha) + (SRT.value * beta) + gamma | ||
| return (10**logE) * u.erg |
There was a problem hiding this comment.
If I understand correctly, this fit is specific to the Nakazato_2013 models (e.g. due to the 20s time range and the shock revival time) and does not make sense for other models? The function also does not appear to be used anywhere, so would it be better to delete it?
python/snewpy/models/extended.py
Outdated
| def extend(self, ts, k = -1., A = None, tau_c = 36. * u.s, alpha = 2.66): | ||
| """Extend supernova model to specific times. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| ts : astropy.Quantity | ||
| Times to add to supernova model. | ||
| k : float | ||
| Power law factor (default: -1) | ||
| A : astropy.Quantity | ||
| Normalization factor (default: None) | ||
| tau_c : astropy.Quantity | ||
| Exponential decay characteristic timescale (default: 36 s) | ||
| alpha : float | ||
| Exponential decay factor (default: 2.66) | ||
| """ | ||
| for t in ts: | ||
| self.time = np.append(self.time, t) |
There was a problem hiding this comment.
Should we enforce here that the times in ts are larger than base_model.time[-1]?
Or alternatively, if we’d like to include a larger time range in ts (e.g. to compare the “true” luminosity with the “extended” luminosity at early times), I wonder whether it might be preferable to keep the “true” and “extended” data points separate. If we just append the extended points to self.time (here) and self.luminosity (below), it’s more difficult to plot the true/extended luminosity separately and it’s easy to end up with plotting artifacts, like in this example from the demo notebook:
There was a problem hiding this comment.
I think @dveyrat's idea was that the ts argument would be a superset of the internal model times, but we can internally force it to drop any times that already exist in the model rather than make the user track which times are present in the model and which are not.
There was a problem hiding this comment.
I’d … probably err on the side of not making the user keep track of such things; but I can also see how it might be lead to confusion if we try to be “too smart” and drop entries when the user might not expect that. 🤔
Maybe we should keep it as is for now, merge, and ask people to test a beta release? And then we can still change it based on feedback.
Co-authored-by: Jost Migenda <[email protected]>
Co-authored-by: Jost Migenda <[email protected]>
Co-authored-by: Jost Migenda <[email protected]>
Co-authored-by: Jost Migenda <[email protected]>
Co-authored-by: Jost Migenda <[email protected]>
There was a problem hiding this comment.
Thanks for resolving these @sybenzvi! I’m pretty happy with this now; there’s still an open UX question, but I think we can go ahead, merge this as is (once the tests have succeeded) and ask people for feedback on that during beta testing.
|
Thanks @JostMigenda , we can figure out the interface later. After your merger I did update the |
Adds extended supernova model class which models neutrino luminosity during the cooling tail.