Boost factor for gravitational-wave inspiral - #861
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds ChangesGravitational-wave inspiral support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BinarySetup
participant DumpSetup
participant GWInspiral
participant MergerAnalysis
BinarySetup->>GWInspiral: set Nstar_gw and iext_gwinspiral
DumpSetup->>GWInspiral: store Nstar_gw in dump state
GWInspiral->>GWInspiral: apply gw_boostfac to force vectors
MergerAnalysis->>GWInspiral: read Nstar_gw
GWInspiral-->>MergerAnalysis: provide star particle ranges
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/moddump_binary.f90 (1)
210-218: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not overwrite a requested GW force with corotation.
When both prompts return true, Line 213 selects
iext_gwinspiral, then Line 217 replaces it withiext_corotate. The resulting dump silently omits gravitational radiation reaction.Reject this combination or implement a combined external-force mode.
Proposed guard
call prompt('Do you want to transform to a corotating frame and simulate corotating binary?', use_corotating_frame) call prompt('Do you want to add gravitational radiation reaction?', gwinspiral) + if (gwinspiral .and. use_corotating_frame) then + call fatal('moddump_binary','gwinspiral and corotation cannot be enabled together') + endif + if (gwinspiral) iexternalforce = iext_gwinspiral🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/moddump_binary.f90` around lines 210 - 218, Update the external-force selection in moddump_binary around the gwinspiral and use_corotating_frame prompts so a requested gravitational-radiation force is not overwritten by the corotation branch. Preserve the existing iext_gwinspiral and iext_corotate paths, but in the set_binary flow either reject the both-true combination with ierr handling or route it through a combined mode instead of assigning iext_corotate last. Use the visible symbols gwinspiral, use_corotating_frame, iexternalforce, iext_gwinspiral, and iext_corotate to keep the fix localized.src/main/extern_gwinspiral.f90 (1)
64-73: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnforce one
Nstar_gwrange invariant in both consumers.
Nstar_gwis restored from setup or dump metadata and is used as both a count and an array-bound offset. Both entry points must reject incomplete states and counts that exceednpart.
src/main/extern_gwinspiral.f90#L64-L73: validate a supported(Nstar_gw(1), Nstar_gw(2), nptmass)tuple and requireNstar_gw(1) + Nstar_gw(2) <= npart.src/utils/analysis_NSmerger.f90#L78-L79: apply the same bound beforetrace_comuses the particle slices.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/extern_gwinspiral.f90` around lines 64 - 73, Update the initialization checks in src/main/extern_gwinspiral.f90#L64-L73 and src/utils/analysis_NSmerger.f90#L78-L79 to enforce the same Nstar_gw invariant: accept only supported (Nstar_gw(1), Nstar_gw(2), nptmass) combinations, reject incomplete or inconsistent states, and fail when Nstar_gw(1) + Nstar_gw(2) exceeds npart before any particle slicing or downstream setup uses it. Keep the existing scenario-specific paths in extern_gwinspiral and apply the same bound check in analysis_NSmerger before trace_com runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/extern_gwinspiral.f90`:
- Around line 64-73: Update the initialization checks in
src/main/extern_gwinspiral.f90#L64-L73 and
src/utils/analysis_NSmerger.f90#L78-L79 to enforce the same Nstar_gw invariant:
accept only supported (Nstar_gw(1), Nstar_gw(2), nptmass) combinations, reject
incomplete or inconsistent states, and fail when Nstar_gw(1) + Nstar_gw(2)
exceeds npart before any particle slicing or downstream setup uses it. Keep the
existing scenario-specific paths in extern_gwinspiral and apply the same bound
check in analysis_NSmerger before trace_com runs.
In `@src/utils/moddump_binary.f90`:
- Around line 210-218: Update the external-force selection in moddump_binary
around the gwinspiral and use_corotating_frame prompts so a requested
gravitational-radiation force is not overwritten by the corotation branch.
Preserve the existing iext_gwinspiral and iext_corotate paths, but in the
set_binary flow either reject the both-true combination with ierr handling or
route it through a combined mode instead of assigning iext_corotate last. Use
the visible symbols gwinspiral, use_corotating_frame, iexternalforce,
iext_gwinspiral, and iext_corotate to keep the fix localized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e0c085b-25ca-4d93-95bf-9bc16a0dad6f
📒 Files selected for processing (6)
src/main/extern_gwinspiral.f90src/main/externalforces.f90src/setup/setup_binary.f90src/utils/analysis_NSmerger.f90src/utils/moddump_binary.f90src/utils/moddump_sinkbinary.f90
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/moddump_binary.f90`:
- Around line 309-312: Update the gwinspiral count assignments in the moddump
binary handling so Nstar_gw follows the particle-array order used by Case 8: set
Nstar_gw(1) to nstar2 and Nstar_gw(2) to nstar1. Do not reorder particles;
preserve the existing storage layout while ensuring get_gw_force_i receives the
correct group boundary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d77b6989-c3b1-48c4-9980-2e1f3014c35f
📒 Files selected for processing (7)
src/main/extern_gwinspiral.f90src/main/externalforces.f90src/setup/setup_binary.f90src/utils/analysis_NSmerger.f90src/utils/moddump_binary.f90src/utils/moddump_binarystar.f90src/utils/moddump_sinkbinary.f90
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/externalforces.f90
- src/utils/moddump_binarystar.f90
- src/utils/moddump_sinkbinary.f90
|
build failure on binary is because new options should not in general be compulsory in the .setup file (and thus require a rewrite of old .setup files) can fix with read_inopt(..., default=gwinspiral) which will give a default value if not found in the .setup file |
|
for the nstar1 and nstar2 stuff, this is kind of horrid as it uses the particle ids to "remember" which star the particle belonged to in the first place so as to apply the radiation reaction from a two body problem to the entire collection of particles belonging to "star 1" and "star 2". At worst we should at least use the iorig array here as a better track of the original particle identities (e.g. would work across MPI threads). In general it'd be nicer to think of how to apply gravitational wave emission in a less hokey way... for example we already have routines to compute the emission by computing the total time varying quadrupole moment , presumably wouldn't be too hard to apply a backreaction force based on that Final suggestion would be if setting gwinspiral to true then you should automatically turn on the gravitational wave strain calculation in the .in file (gravitationalwaves= T) so you get the "actual" gravitational wave emission calculated in the .ev file for comparison) |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/extern_gwinspiral.f90 (1)
307-308: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the inverted
nptmassfallback condition.When
nptmass == 1,Nstar_1is present, andNstar_2is absent, Line 314 testsnptmass > 1. The valid one-sink-plus-gas case therefore reaches the error path. The same condition can misclassify a two-sink header with onlyNstar_2missing.Proposed fix
- elseif (nptmass > 1 .and. ierr2 /= 0) then + elseif (nptmass == 1 .and. ierr1 == 0 .and. ierr2 /= 0) then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/extern_gwinspiral.f90` around lines 307 - 308, Correct the fallback condition in the nptmass handling near the Nstar_1/Nstar_2 extraction calls: use the one-sink condition when Nstar_1 is available and Nstar_2 is absent, and ensure the two-sink case with a missing Nstar_2 is not misclassified. Preserve the existing error path for genuinely invalid headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/extern_gwinspiral.f90`:
- Around line 307-308: Correct the fallback condition in the nptmass handling
near the Nstar_1/Nstar_2 extraction calls: use the one-sink condition when
Nstar_1 is available and Nstar_2 is absent, and ensure the two-sink case with a
missing Nstar_2 is not misclassified. Preserve the existing error path for
genuinely invalid headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 54c41183-c6a8-4adf-a336-0a62c8875d6d
📒 Files selected for processing (1)
src/main/extern_gwinspiral.f90
|
Thanks for the comments, Daniel. The reason why I looked into this is because when doing large mass-ratio CEs or stellar mergers, the transition from a pseudo-Keplerian orbit to a dynamical plunge-in could take a long time, wasting loads of CPU time. And so, following what the AREPO folks have been doing for CE and white-dwarf merger simulations (Pakmor+21, https://arxiv.org/abs/2103.06277), I have decided to apply a secular damping term to the orbit. The easiest way of doing this seems to be boosting the existing gravitational radiation reaction. Doing this by calculating the time-varying quadrupole moment would be an overkill for my purposes, although it may be useful for someone else. |
2 participants
moddump_binary.moddump_sinkbinary(moddump_sinkbinary is redundant #862).gw_nogw_comparison.mp4
The attached video shows an example application, where the left panel shows the use of the gravitational wave inspiral force with a boost factor of ≈ 4*10^11 to achieve an orbital reduction of da/dt ≈ 1 Rsun / orbit. One could also interpret this as slowing the speed of light by a factor of ≈ 200, because da/dt scales as c^-5.
Summary by CodeRabbit
New Features
Bug Fixes
Changes