-
Notifications
You must be signed in to change notification settings - Fork 8
[ZC-1342]: Disable variable syntax in derivations #489
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
base: master
Are you sure you want to change the base?
[ZC-1342]: Disable variable syntax in derivations #489
Conversation
c4633ae
to
25bcb3d
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 amazing work!! Thanks @slobodan-ilic
return list(range(lower, upper + 1)) | ||
|
||
|
||
def parse_expr(expr, platonic=False): |
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.
<3
scrunch/tests/test_datasets.py
Outdated
# `sum` fails | ||
with pytest.raises(Exception, match='not 1.0'): | ||
_test_sum(targets[0]['foo']) | ||
# with pytest.raises(Exception, match='not 1.0'): |
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.
Should this be deleted? Is it not tested or doesn't fail anymore?
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, since the sum doesn't fail, no need to keep this around. deleted.
'expression': { | ||
'function': '!=', | ||
'args': [ | ||
{'variable': var.url}, # Crunch needs variable URLs! |
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.
Is there somewhere a var variable that isn't used anymore now?
"function": "in", | ||
"args": [ | ||
{ | ||
"variable": var1.url |
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.
And a var1
and var2
}, | ||
{ | ||
'variable': 'caseid' | ||
'var': 'caseid' |
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.
Ok, I see so parse_expr
now outputs the var
term directly. Great!!
# We are in the case of a default fill, replace the -1 with the new | ||
# variable | ||
fill_map["-1"] = {"variable": vars_by_alias[else_case["variable"]]["id"]} | ||
fill_map["-1"] = {"var": else_case["var"]} |
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.
Is there still a vars_by_alias
that's being fetched that we may not need anymore?
|
||
elif isinstance(var_value, str): | ||
for var in variables: | ||
for va, var in variables.items(): |
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.
What a mess is all this code. It's so complex for things that should be much simpler :-(
|
||
# inspect function, then inspect variable, if multiple_response, | ||
# then change in --> any | ||
if 'function' in obj and 'args' in obj: |
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.
We're not making this replacement anymore?
This is because in scrunch they say subvar in mr_var
but in zz9 it is more like any(mr_var, [subvar])
so they need to replace IN
for ANY
methods = {m[1]: m[0] for m in CRUNCH_METHOD_MAP.items()} | ||
functions = {f[1]: f[0] for f in CRUNCH_FUNC_MAP.items()} | ||
|
||
def _resolve_variable(var): |
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.
<3
|
||
if 'var' in fragment: | ||
if 'axes' in fragment: | ||
return "{}[{}]".format(fragment['var'], fragment['axes'][0]) |
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.
uhhh are we parsing this back? I can't remember if we have tests that show that scrunch can understand this. It's been so long 🧓🏽
All derivation expressions should use
{"var": <alias>}
syntax. The{"variable": <varid>}
syntax is deprecated and will not be supported in the fututre.