BUG: fix parameter parsing for Castro and MAESTRO #4828
Conversation
| @@ -1128,16 +1132,6 @@ def _parse_parameter_file(self): | |||
| self.parameters[fields[0]] = fields[1].strip() | |||
| line = next(f) | |||
|
|
|||
| # runtime parameters that we overrode follow "Inputs File | |||
There was a problem hiding this comment.
it's not clear to me why this can be removed
There was a problem hiding this comment.
This is doing the same thing as _parse_cparams() in BoxlibDataset, but doesn't strip the trailing space from p or run v through _guess_pcast().
| @@ -1199,21 +1193,8 @@ def _parse_parameter_file(self): | |||
| fields = line.split(":") | |||
| self.parameters[fields[0]] = fields[1].strip() | |||
|
|
|||
| with open(jobinfo_filename) as f: | |||
There was a problem hiding this comment.
Overall looks great. Although, I don't know that we correctly test for this already, do you have a sample dataset that I could use just for manual checking that this is now doing the right thing ?
Also, if I'm reading the top description right, it sounds like this would qualify as a bug fix and should be back ported. Is that right ?
yut23
force-pushed
the
castro-parameters-cleanup
branch
from
2d2dd30 to
6b89ed4
Compare
|
I did my testing on |
|
There's already a test for the MAESTRO parameters; I'll see if I can add one for Castro shortly. |
yut23
force-pushed
the
castro-parameters-cleanup
branch
from
6b89ed4 to
b1a6abb
Compare
yut23
changed the title
| assert "[*] castro.do_hydro" not in ds.parameters | ||
|
|
||
| # Not modified from default | ||
| assert ds.parameters["castro.pslope_cutoff_density"] == float("-1e+200") |
There was a problem hiding this comment.
I tested this on main and this assert fails there with
E AssertionError: assert -1e+20 == -1e+200
E + where -1e+200 = float('-1e+200')
so I guess this parameter is modified from default ? or maybe I'm missing something ?
There was a problem hiding this comment.
Nope, just a typo on my part. It's fixed now.
Castro (since 18.04), MAESTROeX (since 20.09), and MAESTRO (since 2013) add a prefix `[*]` before runtime parameters that are changed from their defaults. This was being handled by the Dataset subclasses individually, leading to duplicated and inconsistent logic (CastroDataset wasn't stripping the parameter names, leading to duplicates with trailing spaces). This change moves the prefix handling into BoxlibDataset, and removes the prefix so analysis scripts don't need to check for both versions in `ds.parameters`.
yut23
force-pushed
the
castro-parameters-cleanup
branch
from
b1a6abb to
b65b3fb
Compare
neutrinoceros
changed the title
|
I edited the title following the history rewrite so that it matches the commit message. Thanks Eric ! |
|
@yt-fido test this please |
1 similar comment
|
@yt-fido test this please |
PR Summary
Castro (since 18.04), MAESTROeX (since 20.09), and MAESTRO (since 2013) add a prefix
[*]before runtime parameters that are changed from their defaults. This was being handled by the Dataset subclasses individually, leading to duplicated and inconsistent logic (CastroDataset wasn't stripping the parameter names, leading to duplicates with trailing spaces).This PR moves the prefix handling into BoxlibDataset, and removes the prefix so analysis scripts don't need to check for both versions in
ds.parameters.PR Checklist