Skip to content

Commit

Permalink
🩹 Fix parameter expression parsing (#843)
Browse files Browse the repository at this point in the history
* 🩹♻️🧪 Fixed Parameter expression transformation

Solved an issue with partially matching strings in the evaluation of parameters relations would cause issues. 

Example:
The expression:
"100 - $inputs1.s1 - $inputs1.s3 - $inputs1.s8 - $inputs1.s12"
should have been transformed to:
"100 - group.get('inputs1.s1').value - group.get('inputs1.s3').value - group.get('inputs1.s8').value - group.get('inputs1.s12').value"
but instead was transformed to:
"100 - group.get('inputs1.s1').value - group.get('inputs1.s3').value - group.get('inputs1.s8').value - group.get('inputs1.s1').value2"

Since "inputs1.s1" partially matches "inputs1.s12".

Regex compiling moved out of the class, so it only gets executed when the module is loaded.

* 👌 Added missing space in exception text of update_parameter_expression
  • Loading branch information
s-weigand authored Sep 30, 2021
1 parent 38cdb71 commit ca1a75e
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 14 deletions.
25 changes: 12 additions & 13 deletions glotaran/parameter/parameter.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,15 @@ class Keys:
VARY = "vary"


PARAMETER_EXPRESION_REGEX = re.compile(r"\$(?P<parameter_expression>[\w\d\.]+)((?![\w\d\.]+)|$)")
"""A regexpression to find and replace parameter names in expressions."""
VALID_LABEL_REGEX = re.compile(r"\W", flags=re.ASCII)
"""A regexpression to validate labels."""


class Parameter(_SupportsArray):
"""A parameter for optimization."""

_find_parameter = re.compile(r"(\$[\w\d\.]+)")
"""A regexpression to find and replace parameter names in expressions."""
_label_validator_regexp = re.compile(r"\W", flags=re.ASCII)
"""A regexpression to validate labels."""

def __init__(
self,
label: str = None,
Expand Down Expand Up @@ -85,10 +86,10 @@ def __init__(

self._transformed_expression: str | None = None

@classmethod
def valid_label(cls, label: str) -> bool:
@staticmethod
def valid_label(label: str) -> bool:
"""Returns true if the `label` is valid string."""
return cls._label_validator_regexp.search(label) is None and label not in RESERVED_LABELS
return VALID_LABEL_REGEX.search(label) is None and label not in RESERVED_LABELS

@classmethod
def from_list_or_value(
Expand Down Expand Up @@ -264,11 +265,9 @@ def expression(self, expression: str | None):
def transformed_expression(self) -> str | None:
"""The expression of the parameter transformed for evaluation within a `ParameterGroup`."""
if self.expression is not None and self._transformed_expression is None:
self._transformed_expression = self.expression
for match in Parameter._find_parameter.findall(self._transformed_expression):
self._transformed_expression = self._transformed_expression.replace(
match, f"group.get('{match[1:]}').value"
)
self._transformed_expression = PARAMETER_EXPRESION_REGEX.sub(
r"group.get('\g<parameter_expression>').value", self.expression
)
return self._transformed_expression

@property
Expand Down
2 changes: 1 addition & 1 deletion glotaran/parameter/parameter_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ def update_parameter_expression(self):
value = self._evaluator(parameter.transformed_expression)
if not isinstance(value, (int, float)):
raise ValueError(
f"Expression '{parameter.expression}' of parameter '{label}' evaluates to"
f"Expression '{parameter.expression}' of parameter '{label}' evaluates to "
f"non numeric value '{value}'."
)
parameter.value = value
Expand Down
5 changes: 5 additions & 0 deletions glotaran/parameter/test/test_parameter.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,11 @@ def test_update_parameter_group_from_array():
("$1", "group.get('1').value"),
("$1-$2", "group.get('1').value-group.get('2').value"),
("$1-$5", "group.get('1').value-group.get('5').value"),
(
"100 - $inputs1.s1 - $inputs1.s3 - $inputs1.s8 - $inputs1.s12",
"100 - group.get('inputs1.s1').value - group.get('inputs1.s3').value "
"- group.get('inputs1.s8').value - group.get('inputs1.s12').value",
),
],
)
def test_transform_expression(case):
Expand Down

0 comments on commit ca1a75e

Please sign in to comment.