search

BlueBrain / nmodl

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

Fix implicit argument handling #1325

Closed JCGoran closed 1 week ago

JCGoran commented 2 weeks ago

at_time, an internal NEURON function, requires an instance of NrnThread*, which is implicit in the mod file, but was previously called with a non-existing argument name (parent function had NrnThread* _nt, while the call to at_time had passed nt).

Note that I am slightly annoyed at the possible fixes here: in NEURON codegen, all of the arguments passed to functions apparently start with an underscore, while for coreNEURON codegen all of them start without an underscore. This suggests a couple of possible fixes:

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.37%. Comparing base (c8d0833) to head (e740c87).

Files Patch % Lines
src/codegen/codegen_neuron_cpp_visitor.cpp 50.00% 5 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1325 +/- ## ======================================= Coverage 85.37% 85.37% ======================================= Files 178 178 Lines 13458 13458 ======================================= Hits 11490 11490 Misses 1968 1968 ```

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

bbpbuildbot commented 2 weeks ago

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

Status and direct links:

bbpbuildbot commented 2 weeks ago

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

Status and direct links:

1uc commented 2 weeks ago

Please check how widely _nt is used in VERBATIM code. Then we can decide if switching to nt is a good idea.

JCGoran commented 2 weeks ago

@1uc let's just say the answer is "non-zero". Can we change coreNEURON codegen instead then (i.e. use option 2 from the description)?

1uc commented 2 weeks ago

You might want to check neurodamus-models before changing the tests, BBP has written MOD files that consist mostly of VERBATIM blocks. I assume they work with CoreNEURON (and NEURON). Therefore, there's a good chance that they use both _nt and nt depending on which simulator is targeted.

JCGoran commented 2 weeks ago

Using both grep -RiIl 'verbatim' | xargs grep '\<_nt\>' and directly grep -RiI '\<nt\>' reveals no matches in neurodamus-models; any other repos that could potentially be problematic? Otherwise, if there's too many things that could potentially break, I'll go back to the drawing board.

bbpbuildbot commented 2 weeks ago

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

Status and direct links: