Fixing comenv SN parameters, adding new wind/remnant/ppisn options#770
Conversation
Created maximum wall time argument
Fixed Nan's apparently.
|
Something is still wrong with the tests I use locally to test this version of COSMIC. However, I'm having a hard time determining the issue by myself, and the comenv.f bug needs to be addressed, so I wanted to create the PR, and we can continue to test it before merging it. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #770 +/- ##
===========================================
- Coverage 86.91% 77.43% -9.48%
===========================================
Files 40 51 +11
Lines 25542 27670 +2128
Branches 0 952 +952
===========================================
- Hits 22198 21424 -774
- Misses 3344 5953 +2609
- Partials 0 293 +293 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…of these modify the contents of this PR
adding in some tiny modifications that allow cosmic-pop to run
TomWagg
left a comment
There was a problem hiding this comment.
This looks great @xevra! Not sure what @katiebreivik thinks here but I think it could be nice to make some of these things more flexible to the user. E.g. for Fryer+22 I think making f_mix a parameter that the user can specify would be great. And the 0.33 multiplier for the winds would be nice to expose to the user too.
I'll take a closer look at the PISN and the CE bit too after either it's an option to make this more flexible, or you both decide it's not necessary :)
I think bigger picture on the winds (not relevant to this specific PR), I think it would be very cool to make the whole thing modular and allow users to swap in different wind prescription for different phases (e.g. have WR winds and AGB winds and LBV winds with different options). At the same time @katiebreivik has pointed out that COSMIC-METISSE may make this obsolete anyway so maybe not an area we need to prioritise 🤷🏻
|
@TomWagg I would love to add options for f_mix and the critical core mass for the Fryer+2022 prescription. However, I don't really know how to add inifile options that don't already exist. Do you have a list of steps that would be needed in order to accomplish this? |
|
Great! Yes I actually very recently created a docs page for this, so you can be the first to test it out! :) https://cosmic-popsynth.github.io/COSMIC/pages/develop/new-settings.html Let me know if anything is unclear, happy to help |
|
Finally got to this @TomWagg I tried to add the relevant options, but it's hung up on "positive_values". Also, I updated the testing Params.ini file, but don't know how to update the hdf5 files. I'm familiar with h5py, but the structure is opaque to h5ls and similar methods. Have any suggestions? Thanks, |
|
One more question: Do you have any idea how to enforce the Fryer+2022 critical core mass parameter? I.e. If the core is above this value, explosion yields a black hole. If below, neutron star. The remnant mass prediction is implemented here, but we're still using mxns to actually decide what kind of remnant we have, right? So the question would be how to use the core mass rather than the total mass of the stellar object to make that determination when remnant=7 is set. |
|
Nice job @xevra!
def add_setting_to_test_files(setting_name, setting_value):
initC = pd.read_hdf("src/cosmic/tests/data/initial_conditions_for_testing.hdf5", key="initC")
initC[setting_name] = setting_value
initC.to_hdf("src/cosmic/tests/data/initial_conditions_for_testing.hdf5", key="initC", mode="a")
initC = pd.read_hdf("src/cosmic/tests/data/kick_initial_conditions.h5", key="initC")
initC[setting_name] = setting_value
initC.to_hdf("src/cosmic/tests/data/kick_initial_conditions.h5", key="initC", mode="a")
# this doesn't check if the setting is already in the file, ensure no duplicates
with open("src/cosmic/tests/data/Params.ini", "a") as f:
f.write(f"\n\n{setting_name} = {setting_value}\n\n") |
|
I seem to have fixed the default values macro! Thanks for the tips! Also updated the pandas table. |
…on highlights for new settings
KB final small docs/test updates.
|
heads up @TomWagg: I'm going to dismiss the requested changes since I'm hoping to clean up PRs waiting in line for this one. I confirm that they've been incorporated! |
The changes have been incorporated but @TomWagg is away and @katiebreivik only has time today to roll through the waiting PRs.
This pull request:
Please check my treatment of the disruption event in comenv.f