Add kilonova event source type - #445
Conversation
|
The remaining CI failures seem unrelated to the kilonova changes. The failing tests are in the lens/profile and plotting parts. I also added redback to requirements.txt because the CI environment did not have it installed, and the new kilonova model depends on it. One concern is that kilonova is an optional source type, so I wonder whether redback should be handled as an optional dependency instead of being required for all SLSim users. Please let me know which approach you prefer. |
|
Yes, I think I can fix the CI with my big change all at once |
97907ee to
e648e85
Compare
sibirrer
left a comment
There was a problem hiding this comment.
Thank you very much!
I could resolve the merge conflict with the new refactoring. I have some minor request before the merge
| pyyaml | ||
| matplotlib | ||
| redback>=1.17.0 | ||
| colossus |
There was a problem hiding this comment.
| colossus |
| speclite | ||
| pyyaml | ||
| matplotlib | ||
| redback>=1.17.0 |
There was a problem hiding this comment.
this is a very specific package only for kilonovae. Might be better to have it only in the test_requirements.txt
| :param model_name: The kilonova light curve model to be used. If not provided, | ||
| the default model is the MOSFiT-based kilonova model. | ||
| :type model_name: str | ||
| :param ejecta_mass: Ejecta masses for the kilonova components. |
There was a problem hiding this comment.
can you provide the units?
| :type model_name: str | ||
| :param ejecta_mass: Ejecta masses for the kilonova components. | ||
| :type ejecta_mass: array-like or None | ||
| :param ejecta_velocity: Ejecta velocities for the kilonova components. |
| :type ejecta_velocity: array-like or None | ||
| :param opacity: Opacities for the kilonova components. | ||
| :type opacity: array-like or None | ||
| :param temperature_floor: Temperature floors for the kilonova components. |
| for name, values in parameter_groups.items(): | ||
| if values is None: | ||
| raise ValueError(f"{name} must be provided.") | ||
| if len(values) != 3: |
There was a problem hiding this comment.
why is this test here? Why are there three values? Can you explain what you test here in the code?
| self._model_name = model_name | ||
| self._mag_zpsys = mag_zpsys | ||
| self._modeldir = modeldir | ||
| self._kwargs_kilonova = kwargs_kilonova or {} |
There was a problem hiding this comment.
is this proper python or what does the 'or' do here?
| times = self._lightcurve_time | ||
|
|
||
| # Use the sncosmo band-name mapping since Redback expects registered filter names. | ||
| provided_band = get_sncosmo_filtername(element) |
There was a problem hiding this comment.
do we need SNCosmo band names? don't understand how this is required
…onova-event # Conflicts: # requirements.txt
| raise ValueError(f"{name} must have three components.") | ||
|
|
||
| self._model_parameters = { | ||
| "mej_1": ejecta_mass[0], |
There was a problem hiding this comment.
perhaps have all these parameters as inputs to this class instead of re-naming them
|
Thank you @Zhengjingyi0823 ! The CI fails because some tests were not updated |
205f7f6 to
15e8d07
Compare
|
From a quick search, perhaps add in the test files where the plots are generated these lines of code: plt.rcParams['text.usetex'] = False |
15e8d07 to
c21b3bc
Compare
|
I added the suggested Matplotlib setting to the plot test file. However, GitHub Actions seems to be experiencing an outage. I tried pushing several times today, but the checks were either stuck in the queue or cancelled. Should I try rerunning them tomorrow? |
sibirrer
left a comment
There was a problem hiding this comment.
Yeah, the GitHub action is either down or we are out of credits. Perhaps we wait and try tomorrow again to merge it
| speclite | ||
| pyyaml | ||
| matplotlib | ||
| matplotlib==3.11.1 |
There was a problem hiding this comment.
do you still need that with the additional fix of not using latex?
I also added trough a separate PR #466 the pit.rcparams parts. Surprised it did not result in a merge conflict or pre-commit cit.
b1854db to
7b455fd
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #445 +/- ##
==========================================
- Coverage 98.62% 98.57% -0.06%
==========================================
Files 112 114 +2
Lines 8733 8822 +89
==========================================
+ Hits 8613 8696 +83
- Misses 120 126 +6
🚀 New features to boost your workflow:
|
|
Ok, looks good now. Just one test that through Source() you can enable the BNS class needs to be tested (two missing lines in the tests) - thank you! |
This PR adds a Redback-based Kilonova light-curve wrapper and integrates it as a new KilonovaEvent source type. It also registers the new "kilonova" keyword in source.py and adds tests for the kilonova model and event-level behavior.