Support SpEC initial data in BBH pipeline #6031
Conversation
knelli2
left a comment
There was a problem hiding this comment.
Have you tested this out yet?
support/Pipelines/Bbh/Inspiral.py
Outdated
| # Here we have to deal with SpEC storing the initial data parameters in a | ||
| # perl script (yay!). We evaluate the perl script, emit the variables as | ||
| # YAML, and load that into Python as a dictionary. |
There was a problem hiding this comment.
Since we are technically added a dependency on perl now, should we document that somewhere? I'm not sure if it's safe to assume that all systems will have perl in the default PATH (though they should...)
| ) | ||
|
|
There was a problem hiding this comment.
Can't reach with github comments, but in this docstring, add that the ID_INPUT_FILE_PATH can either be to a SpECTRE ID input file or a SpEC ID_Params.perl file
support/Pipelines/Bbh/Inspiral.py
Outdated
| ] | ||
| perl_script = [ | ||
| f'do "{id_params_file.resolve()}";', | ||
| "print YAML::Dump({ " |
There was a problem hiding this comment.
Is this part of the core perl install? I don't know how easy it is to install perl modules locally. It may be easier to parse the file ourselves rather than rely on a perl module. We then also don't have to worry about calling into perl from a python script :)
There was a problem hiding this comment.
I don't think so? I just tried on my local machine a test.pl that had only use YAML and it complained about a missing module. This also does warn against using the YAML module if we were going to use a module.
There was a problem hiding this comment.
Ok I wanted to avoid parsing perl syntax with regex because I hate that too :D I'll see what can be done. (this code isn't the most critical in the big picture either, I'd like to delete it again in sooner or later)
There was a problem hiding this comment.
Could you just read the file in as a string, do some replacements like you're already doing and use a yaml parser in python?
There was a problem hiding this comment.
Convert perl syntax to yaml or json? Yea good idea
There was a problem hiding this comment.
Just rewrite all of SpEC to use yaml. Easy ;) :D
There was a problem hiding this comment.
Ok done (@knelli2's suggestion, not @nilsdeppe's 😄)
There was a problem hiding this comment.
Just rewrite all of SpEC to use yaml. Easy ;) :D
Actually that's kind of what we're doing ;)
Proposed changes
Allows to start a BBH evolution from SpEC initial data like this:
Closes #6020.
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