-
Notifications
You must be signed in to change notification settings - Fork 189
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
Add skeleton BBH pipeline #5546
Conversation
b85a456
to
f72bf29
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.
I think this is a great start! We'll definitely need to tune input file parameters and such, but that can be done as we go.
@click.option( | ||
"--mass-ratio", |
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.
How do you feel about using another yaml to specify these? As we add more and more capability, having to write a new click option for every nob we want to tweak will probably get pretty annoying. This could simplify the command down to
spectre bbh generate-id InitialParams.yaml
where InitialParams.yaml
is
mass-ratio: 1.5
separation: 16
orbital-angular-velocity: 0.01
polynomial-order: 7
and then all the other parameters we didn't specify will be default.
This suggestion applies to all phases of the BBH pipeline.
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.
Ohh no, adding another "meta" input file seems like it could be a disaster :D I'd rather keep the knobs here down, and even remove the hp refinement options once we have AMR. If you want full control over options you can write your own input file and run it. The steps in the pipeline still work if you're starting them from a simulation that you didn't run with the pipeline.
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 would be fine with adding an option to supply the CLI option from a configuration file rather than on the command line if you want. We already do something like that with the user config file and the status CLI. It's just an alternative way of passing the options to the CLI. There's a pretty generic way to do that with click.
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.
Yeah I think that would be nice.
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.
Did you want to make this change now or in a follow up 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.
I wasn't planning to do this right away, since I don't have a use case right now
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 agree, could be nice, first need to see how all this works in practice :)
c8eb100
to
988bc81
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.
This is a great start!
@click.option( | ||
"--mass-ratio", |
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.
Did you want to make this change now or in a follow up PR?
Fixed the MPI failure (I hope) |
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 didn't read it all in fine detail, but this seems like a good way to structure and organize all this.
@click.option( | ||
"--mass-ratio", |
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 agree, could be nice, first need to see how all this works in practice :)
For some reason this broke, maybe some package dependencies changed upstream. Disabling recommended packages fixed it.
Clang 15 on M2 macOS also fails to instantiate this class.
Rebased |
Proposed changes
Tools to
generate-id
>start-inspiral
>start-ringdown
. No automation yet and barely any smart parameter choices, but this should provide a good starting point to flesh out the transition from inspiral to ringdown. See #5021 for details on future plans.Try it like this:
(this won't really work yet because the inspiral won't finish properly, but that's #4600)
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