Skip to content
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.

Sign up for GitHub

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

Allow relbin's fiducial parameters to not be floats #4987

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow relbin's fiducial parameters to not be floats #4987

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions pycbc/inference/models/relbin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,11 +655,23 @@ def extra_args_from_config(cp, section, skip_args=None, dtypes=None):
)

# get fiducial params from config
fid_params = {
p.replace("_ref", ""): float(cp.get("model", p))
for p in cp.options("model")
if p.endswith("_ref")
}
fid_params = {}
for p in cp.options("model"):
if p.endswith("_ref"):
val = cp.get("model", p)
if val == '':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a couple places now where we try to do this str -> something python conversion. Maybe we should centralize this now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make sense. This should probably be added to the workflow config parser code, maybe as an kwarg to the various get_opt functions. Do you agree? We could also try to be more sophisticated. The code I wrote above won't distinguish between ints and floats, which can be a problem for some things (numpy functions will raise an error if you try to pass in floats where it expects ints, for example). We could try to follow the Python conventions; e.g., '5' -> int; '5.' -> float.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if we add it to a parser, it should be the base InterpolatingConfigParser, but yeah, that approach makes sense, e.g. extend get_opt

# means had "param =", indicating a boolean that should be
# True
val = True
else:
# try casting to float
try:
val = float(val)
# will get a value error if val is a string; just keep
# as a string in that case
except ValueError:
pass
fid_params[p.replace("_ref", "")] = val

# add optional params with default values if not specified
opt_params = {
Expand Down
Loading