Fix cosmic #694
Fix cosmic #694
Conversation
|
It looks like we are running into some changes with the kicks that don't agree with the previous version. We think this is probably because the periastron check is now leading the binary that is tested from the Pfahl kick test is now not hitting a second kick because there is a merger that happens with the new check. @MathieuVenet is going to check this out but it will probably be on a week long timescale, so hold off until we hear from him @michaelzevin. |
| @@ -2449,29 +2454,27 @@ SUBROUTINE evolv2(kstar,mass,tb,ecc,z,tphysf, | |||
| * | |||
| evolve_type = 8.0 | |||
|
|
|||
| age = tphys - epoch(1) | |||
There was a problem hiding this comment.
@MathieuVenet can you explain to me what this line is doing and why it was changed, as well as the line that reads in aj(1) in hrdiag?
There was a problem hiding this comment.
Originally this line is supposed to define the current evolution time as an input for the hrdiag function. However, when the comenv function is called a new variable aj(1) (and (aj(2)) is defined. This variable is a new evolution time much shorter that the one before the comenv function is called.
When defining the current evolution time, and not this new value, as an input for the hrdiag function, an issue arises.
In such cases, luminosity is computed by the lgbtf function, which relies on this age parameter. If the age is too large, the function encounters a negative value raised to a specific power, which Fortran cannot process, resulting in a NaN. This NaN then propagates through key parameters such as the stellar radius, system separation, and other essential quantities, leading to erroneous results.
To resolve this issue, I removed the redundant redefinition of the evolution time, as it is unnecessary for the time step calculation. Instead, I modified hrdiag to use the time variable aj as defined by the comenv function, ensuring numerical stability and accurate computations.
|
@MathieuVenet can you copy the kick test from the email here and your description about why the kick test failed? I left one clarifying question but other than that this PR looks good. The one thing we'll have to do though is update the CI so the kick test doesn't fail. Although it looks like there might be other CI things that are failing with the current dev version. @katiebreivik, thoughts on this? |
|
For the kick : I have investigated the reason why the kick test failed during the execution of the test_evolve.py function. The issue arises due to the collision test within R.L.O.F. In my case, the system merges during the R.L.O.F. phase, resulting in only one supernova. This explains the changes observed in the kick values. To better illustrate this, I have attached two screenshots showing the outputs for both the standard evolution and the evolution with my modifications. In both screenshots, you will find key evolution parameters and the associated kick_info. Notably, the vsys_2_total value changes from the expected 17 to 0, simply because, in the second case, there is only one supernova and no binary disruption because of the merger.
|
|
Hey @MathieuVenet and @michaelzevin I think the only thing that isn't working is the natal kick check for the pfahl model. The way we fix this is to find a binary that does disrupt, then log it's ejection velocity and hard code the comparison as is done in the test right now. We'll also need to resave data over the current kick data stored in |
|
Thanks @katiebreivik! @MathieuVenet, would you like to take a crack at updating these test functions for that this passes the CI? |
|
Although it also seems like the current development version isn't passing the CI either. |
Yeah this is because I need to fix the Docker image again. I think the tests do pass. |
|
Hi, |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #694 +/- ##
===========================================
- Coverage 86.91% 78.56% -8.35%
===========================================
Files 40 48 +8
Lines 25542 27046 +1504
Branches 0 841 +841
===========================================
- Hits 22198 21247 -951
- Misses 3344 5527 +2183
- Partials 0 272 +272 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Unit tests all pass now, so will merge this in. Thanks again for your work on this @MathieuVenet!! |
-Check for periastron collision during stable R.L.O.F
-Fixing Nan after the C.E
-Fixing undisturbed system after C.E phase and supernova