-
Notifications
You must be signed in to change notification settings - Fork 15
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
Initial attempt at implementing VERBATIM. #1364
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1364 +/- ##
==========================================
- Coverage 86.75% 86.23% -0.52%
==========================================
Files 180 180
Lines 13669 13760 +91
==========================================
+ Hits 11858 11866 +8
- Misses 1811 1894 +83 ☔ View full report in Codecov by Sentry. |
Logfiles from GitLab pipeline #223048 (:white_check_mark:) have been uploaded here! Status and direct links: |
b986231
to
d30cd25
Compare
b68af04
to
80892d4
Compare
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.
Overall looks good, though I haven't checked how this is used in NEURON tests or modelDB. Is there a reference somewhere how users can access the variables in VERBATIM (like _lget_gbl
)? If not, some docs on how NMODL does it would be great as the main NEURON docs don't really list any rules for the replacement mechanism.
I don't know but I'm not writing one. Users should not use and ideally not know about VERBATIM. It constantly causes severe issues that limit our ability to generate code freely. |
There has never been a promise that use of NEURON internals within mod file VERBATIM blocks would be stable. The assumption is that occasionally VERBATIM can be useful even if intimately tied to a specific version NEURON. An example |
In that case, I think we need to put a huge warning label in the NEURON docs, currently there is some mention of compatibility issues, but it's not nearly prominent enough. Regarding the mod file linked: it doesn't work with this NMODL PR as the |
Fixed & testing that now. It's related to |
Regarding how well it's tested, not very well. It's enough to compile everything we need for the |
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.
Seems to work (w/ some non-verbatim-related tweaks) with the built-in NEURON files, great job!
Edit the idea is to temporarily print the NOCMODL macros, e.g. the MOD file:
generates:
One can see the following happening:
There's one more twist relates to
threadargs
in both NOCMODL and NMODL there's two variations of prototypes: oneinternalthreadargsproto
andthreadargsproto
. In NOCMODL the number and order of the arguments are the same (their types differ slightly). Therefore, in NOCMODL a single macro is sufficient to call both types, i.e.threadargs
works for both types of functions. There is nointernalthreadargs
. In NMODL the two signatures differ significantly and for this reason we need to massage VERBATIM code to determine which string to substitute forthreadargs
.