Use SpEC's eccentricity control #6333
Conversation
|
@sxs-collaboration/spectre-core-devs could one of you take this one? |
nilsvu
force-pushed
the
ecc_control
branch
2 times, most recently
from
65d44ab to
ec26450
Compare
There was a problem hiding this comment.
[not a full review] Seems like some modules need to be fixed. Am I understanding this correctly that we have to have SpEC installed in order to do eccentricity control currently? Should we add an error/warning if somebody tries to use it but their build wasn't configured with SpEC?
|
Yes that's correct. I'll add some more checks for that (edit: done). For independence from SpEC we'll have to port the code over at some point. |
nilsvu
force-pushed
the
ecc_control
branch
3 times, most recently
from
e52c26b to
1697c37
Compare
| the target eccentricity, using SpEC's OmegaDotEccRemoval.py. Currently | ||
| supports only circular target orbits (target eccentricity = 0). |
There was a problem hiding this comment.
Add a check that the eccentricity is in fact 0 until we add support for non-zero ecc
| output=None, # reads from Inspiral.yaml | ||
| ): | ||
| def eccentricity_control(h5_files, id_input_file_path, **kwargs): |
| "Omega0", | ||
| "adot0", | ||
| "D0", |
There was a problem hiding this comment.
Do we want to take this opportunity and move away from the names in SpEC to better names? Like initial_angular_velocity, initial_radial_velocity, initial_separation
There was a problem hiding this comment.
These names are actually used a lot, so I tend towards keeping them.
There was a problem hiding this comment.
Ok I'm fine with this, but we should be aware that if we want to change the names it's better to do it earlier rather than later while we're still building everything. We'll be even more resistant to change in the future once this is being used
| # Call into SpEC's OmegaDotEccRemoval.py | ||
| t, Omega, dOmegadt, OmegaVec = ComputeOmegaAndDerivsFromFile(traj_A, traj_B) |
There was a problem hiding this comment.
Can you also add a comment that despite the function name, this expects arrays of data, and not filenames
| subfile_name_aha_quantities: str = "ObservationAhA.dat", | ||
| subfile_name_ahb_quantities: str = "ObservationAhB.dat", |
There was a problem hiding this comment.
Why not factor these defaults out as well?
There was a problem hiding this comment.
I only factored out the defaults because the lines were too long
|
|
||
| def write_mock_trajectory_data( |
There was a problem hiding this comment.
We already have a class tests/Unit/Helpers/PointwiseFunctions/PostNewtonian/BinaryTrajectories.hpp that can compute low order PN (and Newtonian) trajectories that we use in control system testing. Maybe we should move it to a common location, extend it a bit, and bind it in python to use here (that way we don't rewrite things)
| target_eccentricity == 0.0 | ||
| ), "Only circular orbits are supported for eccentricity control yet." |
| "Omega0", | ||
| "adot0", | ||
| "D0", |
There was a problem hiding this comment.
Ok I'm fine with this, but we should be aware that if we want to change the names it's better to do it earlier rather than later while we're still building everything. We'll be even more resistant to change in the future once this is being used
Proposed changes
Call into SpEC to get eccentricity reduction updates. Replaces the port of an old SpEC script.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments