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
| "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
support/Pipelines/EccentricityControl/EccentricityControlParams.py
Outdated
Show resolved
Hide resolved
| 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
nilsvu
force-pushed
the
ecc_control
branch
2 times, most recently
from
387d5b3 to
e816e2a
Compare
support/Pipelines/EccentricityControl/EccentricityControlParams.py
Outdated
Show resolved
Hide resolved
| "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
nilsvu
force-pushed
the
ecc_control
branch
2 times, most recently
from
6241c9e to
6e5ccaa
Compare
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