Skip to content
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

FUNCTION and PROCEDURE codegen for NEURON #1141

Merged
merged 110 commits into from
Feb 11, 2024
Merged

Conversation

iomaganaris
Copy link
Contributor

@iomaganaris iomaganaris commented Jan 23, 2024

iomaganaris and others added 30 commits November 29, 2023 17:10
@bbpbuildbot

This comment has been minimized.

Copy link
Contributor

@pramodk pramodk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iomaganaris: I went through this, just to be sure that if there are any major questions then I can post them / get them clarified.

Few minor questions/comments otherwise it's OK and on our side we can move forward this.

src/codegen/codegen_neuron_cpp_visitor.cpp Outdated Show resolved Hide resolved
src/codegen/codegen_neuron_cpp_visitor.cpp Outdated Show resolved Hide resolved
src/codegen/codegen_neuron_cpp_visitor.cpp Outdated Show resolved Hide resolved
test/usecases/func_proc_pnt/simulate.py Show resolved Hide resolved
@bbpbuildbot

This comment has been minimized.

Base automatically changed from magkanar/need_setdata to master February 9, 2024 17:48
Copy link

codecov bot commented Feb 10, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (545b7b3) 88.18% compared to head (c3edb59) 88.38%.

Files Patch % Lines
src/codegen/codegen_neuron_cpp_visitor.cpp 94.66% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1141      +/-   ##
==========================================
+ Coverage   88.18%   88.38%   +0.19%     
==========================================
  Files         176      176              
  Lines       12791    12946     +155     
==========================================
+ Hits        11280    11442     +162     
+ Misses       1511     1504       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@pramodk pramodk marked this pull request as ready for review February 10, 2024 12:58
@bbpbuildbot

This comment has been minimized.

@bbpbuildbot
Copy link
Collaborator

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

Status and direct links:

Copy link
Contributor

@pramodk pramodk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

  • I have merged the master and reviewed the changes.
    The only noticeable thing I have changed is in c3edb59
@@ -853,31 +852,19 @@ void CodegenNeuronCppVisitor::print_global_variables_for_hoc() {
     printer->add_line("/* declaration of user functions */");
     for (const auto& procedure: info.procedures) {
         const auto proc_name = procedure->get_node_name();
-        if (proc_name[0] == '_') {
-            continue;
-        }

i.e. removal of check of _ in the function/procedure name. NMODL language doesn't allow it and I believe the info.procedures & info.functions don't contain any methods with _ at the beginning. So I am removing that check.

Also, I checked the test with a GLOBAL variable usage in efe963c and it's working fine.

@pramodk pramodk merged commit 0b39872 into master Feb 11, 2024
20 checks passed
@pramodk pramodk deleted the magkanar/nrn_proc_func branch February 11, 2024 08:30
ohm314 pushed a commit that referenced this pull request May 21, 2024
* Added support for the usage of FUNCTION and PROCEDURES
* hoc and python wrappers registration also done
* Corresponding tests added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants