-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BBH ID: compute initial orbital params from PN #5933
Conversation
These can be used until we have ported them to SpECTRE.
Free up '-e' for use as 'eccentricity'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! A few minor suggestions that I'm not sure are reasonable. Please squash if you decide to do them!
"Time to merger. Specify together with a zero eccentricity to compute" | ||
" initial orbital parameters for a circular orbit." | ||
), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main difference would be in the estimated number of orbits to merger. The current initial data just uses the BBH formula at a fixed distance. We do have an estimate of the number of orbits that includes some correction from the BBH formula, but I don't recall whether it's used at all in current automation. @tiamartineau would know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, thank you! I'm just curious for the future, I don't think this should be worried about in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...ah, ignore my first comment, thought you were talking about the orbits to eccentricity reduction, not merger-- the answer is no, not yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that's @tiamartineau !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though GCC has the tests failing. Please squash!
Proposed changes
Port parts of SpEC's ZeroEccParamsFromPN.py to set initial orbital parameters for circular orbits. Two functions from SpEC are just imported, and should be modernized later. This allows to generate near-circular ID like this:
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments