-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Predefined unit variables #348
Merged
DimitriPlotnikov
merged 31 commits into
nest:master
from
PTraeder:predefined-unit-variables
May 23, 2017
Merged
Changes from 28 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
89b4b17
Remove prefixes of d,c,da,h and fix typo
PTraeder de12e9d
Add predefined variables for each possible SI unit
PTraeder 8aa9053
Drop NESTMLNumericLiteral from grammar
PTraeder de4cce3
Fix issues arising from removing NESTMLNumericLiteral
PTraeder f83c3a0
Adapt /models to changes:
PTraeder 037f8ca
Change expressionvisitor system to deal with "literal unit" instead o…
PTraeder 01d33ba
Fix some tests
PTraeder 7d4bc16
Check that "Literal Variable" expressions use only SI-unit variables
PTraeder 84433bc
Change for loop from:
PTraeder c84171f
Adapt test case to grammar change.
PTraeder 5d331e6
Add "isPredefined" to VariableSymbol to indicate if it is a predefine…
PTraeder e648429
Drop predefined VariableSymbols from resolved List before testing Var…
PTraeder 96d98f9
Adapt test case.
PTraeder c2a4085
Change for_stmt step notation again.
PTraeder 434b72d
Fix step notation in models
PTraeder 58f07af
Fix variable name.
PTraeder 8e41d58
fix iteration over symbols
PTraeder b9a5d74
Implement helper funciton to convert a unit into the literal represen…
PTraeder a62ae60
Teach various ReferenceConverters to print units as their literal rep…
PTraeder 6dfdca0
Getter for magnitude
PTraeder 81fa348
Teach expressionprinter to print "literal variable" pairs as "literal…
PTraeder bf4dd1b
Remove debug output
PTraeder 6e84db7
Reinstate "step" notation for for loops. Make step mandatory to avoid…
PTraeder 8d03233
Adapt models to mandatory step notation in for loops
PTraeder 3d35b86
fix variable names
PTraeder cb24016
add unit-variable conversion code to one more ReferenceConverter
PTraeder b2281a8
Install infrastructure and a NESTML specific filter for units.
PTraeder aa7b2f3
Use NESTMLUnitFilter to calculate the modification factors for c++ code.
PTraeder ecab368
Merge branch 'master' of https://github.com/nest/nestml into predefin…
PTraeder 409a6c4
Fix indentation
PTraeder 189dd14
Fixed spacing again
PTraeder File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ill fix it in my next pass wherever I find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 considere
as a dimensionless constant, in which case the right-hand side should be1 nS * e / tau_syn_ex
, or assigne
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 thenS
actually comes from.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have opened #356 to discuss conventions around unit notation.