Skip to content

Conversation

slobodan-ilic
Copy link
Contributor

All derivation expressions should use {"var": <alias>} syntax. The {"variable": <varid>} syntax is deprecated and will not be supported in the fututre.

@slobodan-ilic slobodan-ilic requested a review from jjdelc August 5, 2025 16:39
@slobodan-ilic slobodan-ilic force-pushed the ZC-1342-disable-variable-syntax-in-derivations branch from c4633ae to 25bcb3d Compare August 8, 2025 09:14
Copy link
Contributor

@jjdelc jjdelc left a 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):
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

# `sum` fails
with pytest.raises(Exception, match='not 1.0'):
_test_sum(targets[0]['foo'])
# with pytest.raises(Exception, match='not 1.0'):
Copy link
Contributor

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?

Copy link
Contributor Author

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!
Copy link
Contributor

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
Copy link
Contributor

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'
Copy link
Contributor

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"]}
Copy link
Contributor

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():
Copy link
Contributor

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:
Copy link
Contributor

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):
Copy link
Contributor

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])
Copy link
Contributor

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 🧓🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants