Predefined unit variables #348

PTraeder · 2017-04-20T17:24:55Z

Grammar Changes:
[1]
Originally: Expr = NESTMLNumericLiteral
NESTMLNumericLiteral = NumericLiteral ("["type:UnitType"]"|plainType:Name)?;

Now: Expr = NumericLiteral Variable

Reasoning:
The original version allowed users to write expressions such as the right hand sides of:
foo mV = 13 mV
foo2 mV = 10 [mOhm * A]

Where simple units can be used directly and complex ones have to be encased in square brackets.

Additionally, when correcting the unit of an expression by multiplication/division with another unit
users were required to include a literal with every operation such as:

foo3 m/s = longExpressionOfTypeMeter / 1 s
or
foo4 m/s = longExpressionOfTypeMeter * 1 [1/s]

This was deemed clunky and was syntactically close to the notation of guards ( "[condition]" )
It is replaced by the, hopefully, more intuitive and readable versions:

foo mV = 13 mV
foo2 mV = 10 Ohm * A
foo3 m/s = longExpressionOfTypeMeter / s

The models in the /models folder and those involved in tests were modified accordingly

[2]
Originally: FOR_Stmt = "for" var:Name "in" from:Expr "..." to:Expr ("step" step:SignedNumericLiteral)? BLOCK_OPEN Block BLOCK_CLOSE;

Now: FOR_Stmt = "for" var:Name "in" from:Expr "..." to:Expr "step" step:SignedNumericLiteral BLOCK_OPEN Block BLOCK_CLOSE;

Reasoning:
The original production conflicted with the changes introduced in [1], leading to the trailing Expr matching
the entire end of the statement if "step N" notation was provided.
Making the "step N" notation mandatory leads to it being matched correctly in the presence of the other grammar changes.

The models in the /models folder and those involved in tests were modified accordingly

[3]Predefined Variables:
To make the changes in [1] function in the context of NESTML, predefined variables for each possible SI unit are registered.
Since Type names were already protected this does not effectively change much for the user apart from enabling [1]
A test was introduced to verify that every instance of the "NumericLiteral Variable" notation uses
exclusively valid SI units as variables. Any violations of this are reported as parser errors.

[4]Code generation:
To transfer applicable unit information to the generated C++ code, the following approach was chosen:

Declarations
-> foo mV = someRHS
Replaced by double variables.

Expressions
-> someLHS = 3 mA
The original expression is converted to match the implicit multiplication:
-> someLHS = 3 * mA
The unit-variable is replaced by the numeric representation of its magnitude:
-> someLHS = 3 * 0.001

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

replace "n * unit" with "n unit" replace "1 * unit" with "unit" replace "[complexRhsUnit]" with "complexRhsUnit" Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

…f "literal*unit" Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

for n ... m step k to for n ... m : k To resolve clash with changed expression grammar Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

…d Variable. Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

…iableNotDefinedBeforeUse. Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

…tation of its magnitude Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

…resentation Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

… * convertedVariable" Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

… matching as variable Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

PTraeder · 2017-05-02T19:32:08Z

@jougs @heplesser @Silmathoron I would appreciate your feedback on the changes implemented here.

Silmathoron

On the whole, the changes on the units seem interesting.
However, I have 3 questions:

is it possible to declare variables that have the same name as a unit? (e.g. V)
why did some magnitude prefix disappear?
when converting the magnitude, how did you set the conversion to the NEST units? The example someLHS = 3 mA which becomes someLHS = 3 * 0.001 would be problematic if converted to NEST : 3 mA should be 3.10^9 pA

Regarding the step, I really don't like it; I think it goes against the norm of most usual high-level languages...

I did not understand the part about "Predefined Variables"...

Silmathoron · 2017-05-02T19:41:25Z

models/aeif_cond_alpha.nestml

@@ -86,11 +86,11 @@ neuron aeif_cond_alpha_neuron:

    # Impulse to add to DG_EXC on spike arrival to evoke unit-amplitude
    # conductance excursion.
-    PSConInit_E nS/ms = 1.0 nS * e / tau_syn_ex
+    PSConInit_E nS/ms = nS * e / tau_syn_ex


it's a detail, but I would feel more comfortable if units were put at the end, as a general rule (it is more intuitive)

This remark applies throughout the PR

Ill fix it in my next pass wherever I find it.

I think the unit should always go with the quantity it applies to. In this case, tau_syn_ex carries its own unit (ms). The question is if one now should consider e as a dimensionless constant, in which case the right-hand side should be 1 nS * e / tau_syn_ex, or assign e units, i.e., e nS / tau_syn_ex. I think I like the first version better; one might want to look again at the derivation of this expression to see where the nS actually comes from.

Actually, @Silmathoron, this would leave us with "e / tau_syn_ex * 1.0 nS". I would not be too comfortable with that without adding parents. i.e. "(e / tau_syn_ex ) * 1.0 nS" I am not certain these are necessarily improvements.

I agree with @heplesser and also think that 1 nS * e / tau_syn_ex looks cleaner than (e / tau_syn_ex ) * 1.0 nS or similar.

I have opened #356 to discuss conventions around unit notation.

Silmathoron · 2017-05-02T19:45:50Z

src/main/grammars/org/nest/Commons.mc4

@@ -53,13 +53,12 @@ grammar Commons extends de.monticore.types.Types, org.nest.Units {
         | condition:Expr "?" ifTrue:Expr ":" ifNot:Expr
         | FunctionCall
         | BooleanLiteral // true & false;
-         | NESTMLNumericLiteral
+         | NumericLiteral Variable
+	 | NumericLiteral


missing space (yes, I know, this comment is pretty useless...)

This used to be my job some years ago. Good to see someone take over now that I've grown tired of it ;-)

Silmathoron · 2017-05-02T19:47:35Z

src/main/java/org/nest/units/unitrepresentation/SIData.java

@@ -61,7 +61,7 @@
  private static String[] SIUnitsDerivedRaw =
      {"Hz","N","Pa","J","W","C","V","F","Ohm","S","Wb","T","H","lm","lx","Bq","Gy","Sv","kat"};
  private static String[] SIPrefixesRaw =
-      {"da","h","k","M","G","T","P","E","Z","Y","d","c","m","mu","n","p","f","a","z","y"};
+      {"k","M","G","T","P","E","Z","Y","m","mu","n","p","f","a","z","y"};


why did "da","h", "d","c" disappear?

This one is kind of silly: I dont have prefixes to express their higher powers. E.g. if you take hecto^2 you get a magnitude of 10^4 and I cannot convert that into an error message without a generic exponential notation. I have proposed such but until such time as there is a decision on it, these magnitudes can lead to problems.
Do you see any need for them specifically though?

Ok, I see the problem!
I don't think I've ever seen these magnitudes used, except for space and pressure... I have no definite opinion on this, but maybe @jougs or @heplesser will...

I agree, not even sure what "da" stands for. Not entirely happy with "mu", but I guess there is no alternative if we want to stick to standard ASCII.

I think the removal of these units is not a problem in usual neuroscience applications.

Regarding mu: already extended ASCII had µ at position 230, so I see no principle problem with using it. If not, we should rather use u instead of mu to at least make all prefixes consistently one-letter.

Silmathoron · 2017-05-02T19:48:11Z

src/main/java/org/nest/units/unitrepresentation/SIData.java

@@ -170,13 +168,11 @@ private void populateUnitsList(){
    prefixMagnitudes.put("Z",21);
    prefixMagnitudes.put("Y",24);

-    prefixMagnitudes.put("d",-1);


Silmathoron · 2017-05-02T19:48:16Z

src/main/java/org/nest/units/unitrepresentation/SIData.java

@@ -159,8 +159,6 @@ private void populateUnitsList(){
    baseRepresentations.put("mol",mole);
    baseRepresentations.put("A",ampere);

-    prefixMagnitudes.put("da",1);


Silmathoron · 2017-05-02T19:48:53Z

src/test/resources/codegeneration/imperative/iaf_psc_alpha_imperative.nestml

@@ -14,7 +14,7 @@ neuron iaf_psc_alpha_mc_imperative:
    # Membrane time constant in ms
    Tau ms = 10
    # Membrane capacitance in pF
-    C pF = 250
+    Cap pF = 250


Is this because of the symbol for Coulomb?
I think it would be better to replace this by C_m

Yes. I was unsure how to name these cases sensibly. Will fix.

This just looks so strange to me. Why not

C_m = 250 pF

Silmathoron · 2017-05-02T19:51:15Z

src/test/resources/org/nest/nestml/_cocos/derivativeOrderAtLeastOne/valid.nestml

@@ -1,11 +1,11 @@
 neuron iaf_neuron_ode:

  state:
-    V mV
+    Volt mV


Same here, replace with V_m please

PTraeder · 2017-05-03T13:06:02Z

@Silmathoron
Thank you for your feedback!
1- No, the SI unit types are protected.
2- I tried to answer that where you commented on it directly
3- I had a misconception here. Thanks for pointing it out. Will fix in next pass to set NEST specific target magnitudes.

PTraeder · 2017-05-03T13:18:34Z

@Silmathoron P.S. I do not understand your comment regarding the step notation.

Silmathoron · 2017-05-03T13:48:43Z

Most "user-friendly" script languages consider step = 1 if not provided.
As a user, I would find it really annoying to have to specify it every time (especially since it's by far the most common situation)...

PTraeder · 2017-05-03T13:55:18Z

Yes, I understand that. The problem is that the grammar breaks because the token "step", if it is not mandatory, will not be matched in favor of the preceeding Expr symbol. This is because of the changes made to the Expr symbol. (step gets matched as a variable directly following a literal). If a better delimiter can be agreed on this could be made optional again.

Silmathoron · 2017-05-04T06:33:11Z

I don't understand why you cannot make step a protected keyword so that it is not matched...
On the other hand, I don't know if this for loop will be used much, so maybe it's not worth looking into it too much, but if there is a (potentially recurrent) use case, then I really think that step should be made optional again ;)

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

This will add a factor != 1.0 iff the variable is not one of the NEST specific units. Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

PTraeder · 2017-05-10T05:20:37Z

The recent changes implement the functionality adressed by @Silmathoron.
In the generated c++ code, the constant factors will not be 10 to the power of the unit's magnitude.
Instead, the difference between the given unit and the respective NEST base unit (ms,pF,mV,pA,nS,GOhm) is taken.
For example, the expression "1 s" would be converted to "1 * 1000.0"

jougs · 2017-05-10T08:05:03Z

src/test/resources/org/nest/nestml/_cocos/equationsOnlyForStateVariables/valid.nestml

    G ms
  end

  equations:
    G' = e * t
-    V' = -1/Tau * V + 1/C_m
+    Volt' = -1/Tau * Volt + 1/C_m
  end


Rather make this V_m (Volt is the name of the unit. voltage would work here, but V_m is much more common)

jougs · 2017-05-10T08:06:31Z

models/aeif_cond_alpha.nestml

@@ -86,11 +86,11 @@ neuron aeif_cond_alpha_neuron:

    # Impulse to add to DG_EXC on spike arrival to evoke unit-amplitude
    # conductance excursion.
-    PSConInit_E nS/ms = 1.0 nS * e / tau_syn_ex
+    PSConInit_E nS/ms = nS * e / tau_syn_ex


I agree with @heplesser and also think that 1 nS * e / tau_syn_ex looks cleaner than (e / tau_syn_ex ) * 1.0 nS or similar.

jougs · 2017-05-10T08:08:09Z

src/main/grammars/org/nest/Commons.mc4

@@ -53,13 +53,12 @@ grammar Commons extends de.monticore.types.Types, org.nest.Units {
         | condition:Expr "?" ifTrue:Expr ":" ifNot:Expr
         | FunctionCall
         | BooleanLiteral // true & false;
-         | NESTMLNumericLiteral
+         | NumericLiteral Variable
+	 | NumericLiteral


This used to be my job some years ago. Good to see someone take over now that I've grown tired of it ;-)

jougs · 2017-05-10T08:19:53Z

src/main/java/org/nest/units/unitrepresentation/SIData.java

@@ -61,7 +61,7 @@
  private static String[] SIUnitsDerivedRaw =
      {"Hz","N","Pa","J","W","C","V","F","Ohm","S","Wb","T","H","lm","lx","Bq","Gy","Sv","kat"};
  private static String[] SIPrefixesRaw =
-      {"da","h","k","M","G","T","P","E","Z","Y","d","c","m","mu","n","p","f","a","z","y"};
+      {"k","M","G","T","P","E","Z","Y","m","mu","n","p","f","a","z","y"};


I think the removal of these units is not a problem in usual neuroscience applications.

Regarding mu: already extended ASCII had µ at position 230, so I see no principle problem with using it. If not, we should rather use u instead of mu to at least make all prefixes consistently one-letter.

…ed-unit-variables # Conflicts: # src/main/java/org/nest/codegeneration/converters/NESTReferenceConverter.java # src/main/java/org/nest/spl/prettyprinter/LegacyExpressionPrinter.java # src/main/java/org/nest/spl/prettyprinter/SPLPrettyPrinter.java # src/main/java/org/nest/symboltable/symbols/VariableSymbol.java # src/main/java/org/nest/utils/AstUtils.java Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Silmathoron · 2017-05-19T07:34:24Z

src/main/grammars/org/nest/Commons.mc4

@@ -54,7 +54,7 @@ grammar Commons extends de.monticore.types.Types, org.nest.Units {
         | FunctionCall
         | BooleanLiteral // true & false;
         | NumericLiteral Variable
-	 | NumericLiteral
+	       | NumericLiteral


Something went wrong with the indentation fix, here xD

I have no idea what is going wrong here, I fixed the spacing again (and it looked okay on my machine both times).
I removed all whitespaces in the area and re-entered normal spaces. Still does not seem to help it.

It even looks okay in git's online editor. I have no clue.

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

PTraeder added 24 commits April 17, 2017 01:57

Remove prefixes of d,c,da,h and fix typo

89b4b17

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Add predefined variables for each possible SI unit

de12e9d

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Drop NESTMLNumericLiteral from grammar

8aa9053

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Fix issues arising from removing NESTMLNumericLiteral

de4cce3

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Adapt /models to changes:

f83c3a0

replace "n * unit" with "n unit" replace "1 * unit" with "unit" replace "[complexRhsUnit]" with "complexRhsUnit" Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Change expressionvisitor system to deal with "literal unit" instead o…

037f8ca

…f "literal*unit" Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Fix some tests

01d33ba

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Check that "Literal Variable" expressions use only SI-unit variables

7d4bc16

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Change for loop from:

84433bc

for n ... m step k to for n ... m : k To resolve clash with changed expression grammar Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Adapt test case to grammar change.

c84171f

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Add "isPredefined" to VariableSymbol to indicate if it is a predefine…

5d331e6

…d Variable. Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Drop predefined VariableSymbols from resolved List before testing Var…

e648429

…iableNotDefinedBeforeUse. Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Adapt test case.

96d98f9

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Change for_stmt step notation again.

c2a4085

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Fix step notation in models

434b72d

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Fix variable name.

58f07af

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

fix iteration over symbols

8e41d58

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Implement helper funciton to convert a unit into the literal represen…

b9a5d74

…tation of its magnitude Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Teach various ReferenceConverters to print units as their literal rep…

a62ae60

…resentation Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Getter for magnitude

6dfdca0

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Teach expressionprinter to print "literal variable" pairs as "literal…

81fa348

… * convertedVariable" Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Remove debug output

bf4dd1b

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Reinstate "step" notation for for loops. Make step mandatory to avoid…

6e84db7

… matching as variable Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Adapt models to mandatory step notation in for loops

8d03233

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Silmathoron reviewed May 2, 2017

View reviewed changes

PTraeder added 4 commits May 10, 2017 06:45

fix variable names

3d35b86

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

add unit-variable conversion code to one more ReferenceConverter

cb24016

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Install infrastructure and a NESTML specific filter for units.

b2281a8

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

Use NESTMLUnitFilter to calculate the modification factors for c++ code.

aa7b2f3

This will add a factor != 1.0 iff the variable is not one of the NEST specific units. Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

jougs reviewed May 10, 2017

View reviewed changes

PTraeder added 2 commits May 19, 2017 06:58

Fix indentation

409a6c4

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

PTraeder mentioned this pull request May 19, 2017

Define preferred notation of units in models #356

Closed

Silmathoron reviewed May 19, 2017

View reviewed changes

Fixed spacing again

189dd14

Signed-off-by: Philip Traeder <philip.traeder@rwth-aachen.de>

DimitriPlotnikov merged commit 4925a45 into nest:master May 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Predefined unit variables #348

Predefined unit variables #348

PTraeder commented Apr 20, 2017 •

edited

PTraeder commented May 2, 2017

Silmathoron left a comment

Silmathoron May 2, 2017

PTraeder May 3, 2017

heplesser May 8, 2017

PTraeder May 9, 2017 •

edited

jougs May 10, 2017

PTraeder May 19, 2017

Silmathoron May 2, 2017

jougs May 10, 2017

Silmathoron May 2, 2017

PTraeder May 3, 2017 •

edited

Silmathoron May 3, 2017

heplesser May 8, 2017

jougs May 10, 2017

Silmathoron May 2, 2017

Silmathoron May 2, 2017

Silmathoron May 2, 2017

PTraeder May 3, 2017

heplesser May 8, 2017

Silmathoron May 2, 2017

PTraeder commented May 3, 2017

PTraeder commented May 3, 2017

Silmathoron commented May 3, 2017

PTraeder commented May 3, 2017

Silmathoron commented May 4, 2017

PTraeder commented May 10, 2017

jougs May 10, 2017

jougs May 10, 2017

jougs May 10, 2017

jougs May 10, 2017

Silmathoron May 19, 2017

PTraeder May 20, 2017

PTraeder May 20, 2017

Predefined unit variables #348

Predefined unit variables #348

Conversation

PTraeder commented Apr 20, 2017 • edited

PTraeder commented May 2, 2017

Silmathoron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PTraeder May 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PTraeder May 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PTraeder commented May 3, 2017

PTraeder commented May 3, 2017

Silmathoron commented May 3, 2017

PTraeder commented May 3, 2017

Silmathoron commented May 4, 2017

PTraeder commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PTraeder commented Apr 20, 2017 •

edited

PTraeder May 9, 2017 •

edited

PTraeder May 3, 2017 •

edited