-
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
Single iteration of eccentricity control reduction #6295
Conversation
a = ecout["updated xcts values"]["expansion"] | ||
print("current eccentricity = " + str(ecout["eccentricity"])) | ||
print("updated value for omega = " + str(w)) | ||
print("updated value for expansion = " + str(a)) |
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.
Below this add a CLI command that just calls your function, and where you can pick all the options (like tmin
and tmax
. You can copy this from support/Pipelines/EccentricityControl/EccentricityControl.py
, and we will extend it later.
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.
This is still missing (I didn't see it in the fixup 1
)
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.
oops, this might have gotten deleted during one of my branch switches
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.
save for next PR
a = ecout["updated xcts values"]["expansion"] | ||
print("current eccentricity = " + str(ecout["eccentricity"])) | ||
print("updated value for omega = " + str(w)) | ||
print("updated value for expansion = " + str(a)) |
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.
We will call generate_id()
here to start the next ecc-control iteration. But let's do that in the next PR.
return functions["H4"]["fit result"] | ||
|
||
|
||
def ecc_control_fit( |
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.
Please add a test for the new code in this PR, similar to the general Test_EccentricityControl.py
or Test_PlotTrajectories.py
.
# Suppress specific RuntimeWarnings | ||
warnings.filterwarnings( | ||
"ignore", message="Number of calls to functionhas reached maxfev" | ||
) |
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.
Oh that's cool, I didn't know this exists.
a = ecout["updated xcts values"]["expansion"] | ||
print("current eccentricity = " + str(ecout["eccentricity"])) | ||
print("updated value for omega = " + str(w)) | ||
print("updated value for expansion = " + str(a)) |
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.
This is still missing (I didn't see it in the fixup 1
)
num_obs = len(ObjectA_centers[:, 0]) | ||
|
||
if len(ObjectB_centers[:, 0]) < num_obs: | ||
num_obs = len(ObjectB_centers[:, 0]) |
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.
Please drop all changes to this file.
I think what happened is you copied this script a while ago, some updates were made since then, and now you copied your version back into this one. For example, in this line here you fixed the num_obs
, and I also fixed it in a slightly different way with the min()
function.
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.
ok!
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.
Looks good you can squash everything into a single commit, including these two extra changes. Now only the test is missing.
expansion_from_xcts: orbital parameter we are interested in updating; | ||
optional parameter, if none is provided, defaults to None | ||
tmin: sets starting point for eccentricity reduction script; defaults to | ||
h5_file: file that contains the trajectory data; optional parameter |
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.
delete "optional parameter" (it's not optional)
is provided, defaults to None | ||
id_input_file_path: path to the input file of the initial data run; reads | ||
from Inspiral.yaml, if none provided, defaults to None. | ||
output: represents output of the function; optional parameter |
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.
Please be more specific. Should this be a directory where plots will be written to, or a file path for a plot? I don't remember.
6d24505
to
748a6fb
Compare
) | ||
|
||
def create_yaml_file(self): | ||
try: |
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.
delete the try/except
|
||
# Verify that the HDF5 file was created correctly and contains | ||
# the expected data | ||
def test_h5_file_created_correctly(self): |
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.
I think you don't need this
748a6fb
to
a46e729
Compare
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.
Looks great! Nice job getting this finished.
ab00f87
to
3302a22
Compare
3302a22
to
9dd76b7
Compare
Proposed changes
Added eccentricity control option to Next section of Inspiral.yaml. Options now after the inspiral ends are to either proceed with the ringdown, or choose eccentricity control instead. If eccentricity control is chosen, the output will include current eccentricity, suggested orbital parameter updates, and a pdf of dD/dt, Residual, and coordinate separation plots.
Example command line to run:
spectre bbh start-inspiral --eccentricity-control -L 1 -P 10 -d ./segments ./bbh_id/InitialData.yaml -p reservation=sxs_standing -N 6
Option to test the function quickly without running an actual inspiral:
1.) make a directory
2.) carry in an old bbh_id directory from a past simulation
3.) run
spectre bbh start-inspiral --eccentricity-control -L 1 -P 10 -d ./segments ./bbh_id/InitialData.yaml -p reservation=sxs_standing -N 6
4.) enter "n" when asked to submit job
5.)
cd segments/002_Inspiral/Segment_0000
directory6.) carry in an old BbhReductions.h5 file from a past simulation
7.) cd .. back into segments directory
8.) run
spectre run-next ./002_Inspiral/Segment_0000/Inspiral.yaml
Output will look like this:
and in
/segments/002_Inspiral/Segment_0000
, there will also be an ecc_control.pdf file that looks like this: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.