Support missing namespace in parameter names in design_kw #163
Conversation
|
Can one of the admins verify this patch? |
berland
force-pushed
the
gen_kw_handling
branch
from
41ff95c
to
acf323a
Compare
There was a problem hiding this comment.
I think this is a fair approach and the tests seems adequate 👍 I would like to see an added test for logging if that is the expected behaviour and in addition I think the implementation of rm_genkw_prefix could be simplified a bit.
I'm supporting this direction
| ({"foo:bar": "value"}, {"bar": "value"}), | ||
| ({"foo:bar": "value", "foo:com": "value2"}, {"bar": "value", "com": "value2"}), | ||
| ({}, {}), | ||
| # Duplicates are dropped, a warning is logged. |
There was a problem hiding this comment.
I don't think any warning is logged here...
There was a problem hiding this comment.
Added extra duplicates warning.
semeio/jobs/design_kw/design_kw.py
Outdated
| @@ -116,3 +119,53 @@ def extract_key_value(parameters): | |||
| if errors: | |||
| raise ValueError("\n".join(errors)) | |||
| return res | |||
|
|
|||
|
|
|||
| def rm_genkw_prefix(str_or_dict, ignoreprefixes="LOG10_"): | |||
There was a problem hiding this comment.
Unless I've misunderstood the type of str_or_dict will always be a dict on the first call, and then the string support is used internally on the keys afterwards. I think the implementation would be much cleaner if we utilised this assumption and hid the string-support within the function such that rm_genkw_prefix only accepts dicts of depth one.
There was a problem hiding this comment.
New implementation written - it got a lot better with only dict as input.
berland
force-pushed
the
gen_kw_handling
branch
5 times, most recently
from
6fcec8d
to
5e6bac1
Compare
semeio/jobs/design_kw/design_kw.py
Outdated
|
|
||
|
|
||
| def rm_genkw_prefix(paramsdict, ignoreprefixes="LOG10_"): | ||
| """Remove anything before the first colon in a string, or remove |
There was a problem hiding this comment.
The doc string still need an update, besides that it looks good 👍 Nice clean up of rm_genkw_prefix ;)
rm_genkw_prefix is refactored and improved from fm_pyscal into design_kw. fm_pyscal will not any longer ignore a namespace prefix in the interpolation parameter names, if the user actually supplies a prefix (has probably never happened in practice yet) Update docs for design_kw accordingly
berland
force-pushed
the
gen_kw_handling
branch
from
5e6bac1
to
9addc3a
Compare
First attempt at solving #59
Check first if the behaviour imposed by the added tests is what we want, then assess whether the code achieving it was sufficiently elegant (perhaps it was not..)