Feature/kicks redesign - #18
Merged
Merged
Conversation
This is the source of a major bug that has been in pretty much every version of SSPtools. The fallback fraction is a function of the *initial* progenitor mass, not the final BH mass, as we have been using. This will, unfortunately, not be a simple fix, and will require rethinking how natal kicks are implemented entirely.
While working on kick stuff, where this class became necessary in order to work out some statistics, I realized that this entire thing, which before consisted of the same ODE as the rest of the classes, but stripped of anything not BH related, could be accomplished with some simple math, given that we know the IMF and the IFMR. This class should give (nearly) the same results as the previous, but in a fraction of the time (~1 sec to ~0.02 sec). This allows it to be used in other places without much worry. Note that this *does* some of the init signatures, so it is not entirely backwards compatible
The kick statistics cannot be easily computed now that the kicks are done during the ODE solving directly, not on the BH bins after the fact. However, these are still useful to have. Therefore this commit changes `KickStats` to try to recreate this logic as best as possible. Note that these will be approximate at best, but it is less important now, as they will not be used directly for anything crucial.
This commit changes entirely how the BH natal kicks are handled and applied to the `evolve_mf` classes. Namely, this is the fix from the old incorrect computations based on the final mass, to the correct version using progenitor masses. This removes all kick logic from the after-the-fact BH ejection stage, and instead replaces the largely unused `_frem` map with a new function which calls the relevant retention functions from `kicks`. This means that the kicks are now applied during the creation of the BHs themselves, removing some fraction over time based on the progenitor star masses. A lot of other logic had to change to account for these changes. Notably, the kick stats are no longer as important, and have become more approximate. Relatedly, the meaning of `BH_ret_dyn` has now changed, in a *not backwards compatible* manner, and now represents only the dynamical ejection retention fraction. Also changed is the handling of `f_kick`, which now has the parameters determined once at the start, and the actual used kick params stored.
Also bump version a major release, due to backwards-incompatibility.
Member
Author
|
The tests will need to be entirely rewritten, I imagine |
In IFMRS with a wider range of BH masses (like SEVN, with PISN), we both need to be sure we ignore the destroyed m=0 BHs, and need to make sure any bins constructed for the BHs can hold BHs of mass greater than `m_breaks[-1]`.
nmdickson
marked this pull request as ready for review
May 5, 2026 20:54
Member
Author
|
Testing with GCfit, clusterBH seem to be working, but this has quite a large impact on the kicks and is difficult to know exactly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a major redesign of how BH natal kicks are handled in SSPtools, motivated by a bug that was found during #17.
Originally (and for the entire history of the package) the BH natal kicks were computed (by default) based on fallback fractions which were being incorrectly interpolated from a function of BH-mass vs fallback. In reality, this should be a function of initial progenitor star mass, not BH mass.
This was probably originally done because the kicks were applied after-the-fact (same as the dynamical BH ejections) where you have only the final BH mass bins, and can compute these quantities.
In order to change all kick functions to be based on the initial mass, how the kicks are handled thus had to be changed entirely. Now, rather than computing and applying them after the solver is finished, the natal kicks are instead applied as retention fractions to the amount of BH mass formed at a given timestep.
This is the same way that e.g. NS kicks and the unused
BH_ret_intwere used originally.This means that not all BHs in a given mass bin share the same retention function, and the incorrect fb-interpolation now no longer causes zero BHs in slightly larger BH mass bins to be ejected.
This is a major change under the hood, and comes with various "user-facing" side effects that make this somewhat backwards-incompatible (and thus a new major release).
The natal kick stats
KickStats(stored asemf._kick_stats) are no longer easy to compute (as we no longer kick BHs based on the BH bins, which these stats are a function of). The new way of computing them, by trying to autopsy the final results with and without kicks, are approximate at best, and can show non-physical results based on age and binning effects. Therefore they were made less relevant, and are only provided now as they are interesting to inspect. They should be actually used with caution.To stop needing the kick stats, the definition of the
BH_ret_dynparameter had to change. Now it only represents the retention function of BHs from dynamical ejections, not natal kicks as well. Similarly, theBH_ret_intparameter is entirely removed (replaced by the natal kicks themselves). This was likely actually the original intended use of this parameter, based on its name. This change is not backwards compatible, obviously.The
InitialBHPopulationclass is more important now, as in various places it can be useful to know how many BHs would form in total. To facilitate this, the class is vastly improved, and now based simply on going from IMF through IFMR to BHMF, rather than actually solving a binned ODE. It is much (100x) faster now, but also not backwards compatible.And of course the most obvious change is that new models, run with entirely the same parameters, will have different amounts of final BHs (slightly lower), simply due to the fixes to the natal kicks.