AllenInstitute / MIES

Multichannel Igor Electrophysiology Suite
https://alleninstitute.github.io/MIES/user.html
Other
23 stars 7 forks source link

SweepFormula: Support minus as sign in the parser (and therefore don't require parantheses) #278

Closed t-b closed 4 years ago

t-b commented 5 years ago
•print FormulaExecutor(FormulaParser("-min([0, 1])"))[0]
  -1
•print FormulaExecutor(FormulaParser("+min([0, -1])"))[0]
  0

that is not what you expect. Using 58f24b8a (UTF_SweepFormula: Use module definition and make the functions static, 2019-09-21).

ukos-git commented 5 years ago

Remember that -1 is not numeric but an object

ukos-git commented 5 years ago

Why don‘t you just use (-1)*min(0,1)

ukos-git commented 5 years ago

the actual problem that you describe is simpler than your example.

min(0,-1) currently gives

{
  "min": [
    0,
    1
  ]
}

while

min(0,(-1)) gives

{
  "min": [
    0,
    {
      "-": [
        1
      ]
    }
  ]
}

and your formulas are solvable by parentheses

+min(0,(-1))

{
  "+": [
    {
      "min": [
        0,
        {
          "-": [
            1
          ]
        }
      ]
    }
  ]
}

The simplest solution is encapsulating (-[0-9]) and (+[0-9]) by parenthesis in the preparser but that would interfere with operation evaluation.

A similar approach as for the parentheses evaluation would also be possible.

ukos-git commented 5 years ago

what do you suggest for solving the -1 problems?

ukos-git commented 5 years ago

Personally, I recommend to always encapsulate -1 in brackets.

ukos-git commented 5 years ago

currently failing evaluations:

1+-1

{
  "+": [
    1,
    1
  ]
}

-1*2

{
  "-": [
    {
      "*": [
        1,
        2
      ]
    }
  ]
}

2+-1*3

{
  "+": [
    2,
    {
      "*": [
        1,
        3
      ]
    }
  ]
}

non-first array elements:

[1, 0, -1]

[
  1,
  0,
  1
]

-1 - 1

{
  "-": [
    1,
    1
  ]
}
t-b commented 5 years ago
[0, 1, 2] [3, 4, 5]

evaluates to

[
  0,
  1,
  2,
  [
    3,
    4,
    5
  ]
]

which is a bit suprising.

ukos-git commented 5 years ago

That is a bug in the parser. Should we track all issues of the parser here?

It should be of no particular relevance to our use scenario but definitively needs a fix.

ukos-git commented 5 years ago

I think, such checks for invalid syntax should go to the preparser. But we could probably also catch it in the parser.

The check for the above example would be that GrepString(str, "\]\s*\[") is invalid

t-b commented 5 years ago

@ukos-git Collecting them here sounds good!