search

BlueBrain / nmodl

Code Generation Framework For NEURON MODeling Language
https://bluebrain.github.io/nmodl/
Apache License 2.0
53 stars 15 forks source link

Fix bug with state named 'is'. #1263

Closed 1uc closed 4 months ago

1uc commented 4 months ago

Sympy doesn't tolerate variable names called is.

1uc commented 4 months ago

Fixes #1258.

bbpbuildbot commented 4 months ago

Logfiles from GitLab pipeline #211054 (:no_entry:) have been uploaded here!

Status and direct links:

bbpbuildbot commented 4 months ago

Logfiles from GitLab pipeline #211065 (:white_check_mark:) have been uploaded here!

Status and direct links:

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.32%. Comparing base (2d9e7c4) to head (0b48310).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1263 +/- ## ======================================= Coverage 86.32% 86.32% ======================================= Files 175 175 Lines 13219 13219 ======================================= Hits 11411 11411 Misses 1808 1808 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bbpbuildbot commented 4 months ago

Logfiles from GitLab pipeline #211096 (:white_check_mark:) have been uploaded here!

Status and direct links:

bbpbuildbot commented 4 months ago

Logfiles from GitLab pipeline #211224 (:white_check_mark:) have been uploaded here!

Status and direct links:

bbpbuildbot commented 4 months ago

Logfiles from GitLab pipeline #211252 (:white_check_mark:) have been uploaded here!

Status and direct links:

JCGoran commented 4 months ago

Will still break if the following keywords are declared as NMODL variables:

'nonlocal', 'except', 'else', 'from', 'not', 'with', 'for', 'or', 'try', 'and', 'class', 'None', 'True', 'async', 'continue', 'break', 'False', 'if', 'return', 'assert', 'await', 'global'

Other than that, a much better solution that what I had in mind.

1uc commented 4 months ago

I understand that it's rather partial solution. Some are tricky: should if be a legal variable name? I don't know and I want to punt. Same for global. I'd be quite happy if it continues to crash for all of those.

The problem is we only have strings. We don't know if a particular substring is a variable or if it's a keyword. We know if a particular string is used as a variable name; but we don't know if a particular occurrence of the string is a variable or a keyword.

JCGoran commented 4 months ago

Re: legality of variable naming, shouldn't NOCMODL be the reference for what is valid and what isn't (however strange the rules may be)? In any case, the variables which NOCMODL doesn't seem to like (out of the ones I listed above) are apparently else not for or and if return assert, while the other ones compile fine, so maybe we can just add those in the list of variables to rename and call it a day for now.

1uc commented 4 months ago

Makes sense. The proper place to fix all of this is when NMODL create the string it sends to SymPy. That string should not contain any troublesome identifiers, i.e. all variable should probably be mangled first; and demangled after SymPy. There we'd have a better understanding of each substring.

1uc commented 4 months ago

I'm surprised class and try would work. Edit: Liberal use of #define makes it work.

JCGoran commented 4 months ago

I'm surprised class and try would work.

NOCMODL seems to define those, which is apparently legal (though it raises a warning on clang): https://godbolt.org/z/sa9qddYTY

bbpbuildbot commented 4 months ago

Logfiles from GitLab pipeline #211535 (:no_entry:) have been uploaded here!

Status and direct links:

1uc commented 4 months ago

It feels very strange to allow C++ keywords as a variable names. It feels even more off to "fix" it by patching the string sent to SymPy, meaning the C++ keywords are returned from SymPy again. There's a better way of doing this by adding an early pass which renames any variable names that are C++ or Python keywords to something safe. Then we don't need to do any trickery with the string to be sent to SymPy.

I get your point of nocmodl being the reference. Since it's all not very clean, I've reduced the number of keywords we sanitize in this unsatisfactory way to a small number of what I believe a plausible variable names. I'd be open to reducing what works even futher to just is and maybe in, to make the bug Koen is seeing go away.

JCGoran commented 4 months ago

Since keeping a list of both C++ and Python keywords is a bit cumbersome, we could add prefix/suffix to all variables internally or something, but we can postpone the details of that discussion for later. For now, as long as this PR fixes the issue, I'm fine with it as a temporary workaround.

bbpbuildbot commented 4 months ago

Logfiles from GitLab pipeline #211937 (:no_entry:) have been uploaded here!

Status and direct links:

bbpbuildbot commented 4 months ago

Logfiles from GitLab pipeline #212210 (:white_check_mark:) have been uploaded here!

Status and direct links: