Fix Newton solver code generation by iomaganaris · Pull Request #696 · BlueBrain/nmodl

iomaganaris · 2021-06-21T16:52:29Z

Fixes Error in code generation of reduced dentate benchmark (1) #691
Add const qualifier to the operator() of functor struct only if there is no external variable being defined inside the function
Temprorary fix for Error in code generation of reduced dentate benchmark (2) #692 by adding #ifdef CORENEURON_BUILD in the code generated for CONSTRUCTOR and DESTRUCTOR
Updated DefUseAnalyzeVisitor to handle cases of LOCAL and GLOBAL` variables similar to this:

NEURON {
    RANGE tau
}
DERIVATIVE states {
    IF (tau == 1) {
    } ELSE {
        LOCAL tau
        tau = 1 + tau
    }
}

In this case tau should be DUState::U for example because as a GLOBAL variable we care about it's usage as a GLOBAL variable and not the LOCAL variable tau which is defined inside the IF-ELSE statement.

bbpbuildbot · 2021-06-21T17:21:41Z

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

Status and direct links:

bbpbuildbot · 2021-06-22T17:07:13Z

Logfiles from GitLab pipeline #9498 (:no_entry:) have been uploaded here!

Status and direct links:

bbpbuildbot · 2021-06-22T17:24:10Z

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

Status and direct links:

codecov-commenter · 2021-06-29T10:02:10Z

Codecov Report

Merging #696 (4599304) into master (d7b82bb) will increase coverage by 0.00%.
The diff coverage is 70.00%.

@@           Coverage Diff           @@
##           master     #696   +/-   ##
=======================================
  Coverage   60.47%   60.47%           
=======================================
  Files         200      200           
  Lines       33283    33345   +62     
=======================================
+ Hits        20128    20167   +39     
- Misses      13155    13178   +23

Impacted Files	Coverage Δ
src/language/templates/visitors/symtab_visitor.hpp	`42.85% <0.00%> (-17.15%)`	⬇️
src/symtab/symbol_table.hpp	`78.57% <ø> (ø)`
src/codegen/codegen_c_visitor.cpp	`63.43% <12.00%> (-0.46%)`	⬇️
src/visitors/defuse_analyze_visitor.cpp	`86.48% <100.00%> (+0.70%)`	⬆️
src/visitors/defuse_analyze_visitor.hpp	`93.33% <100.00%> (-6.67%)`	⬇️
test/unit/visitor/defuse_analyze.cpp	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7b82bb...4599304. Read the comment docs.

bbpbuildbot · 2021-06-29T10:02:30Z

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

Status and direct links:

bbpbuildbot · 2021-07-23T14:22:25Z

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

Status and direct links:

pramodk · 2021-07-26T16:21:39Z

+    // Create complete_block with both variable declarations (done in variable_block) and solver
+    // part (done in functor_block) to be able to run the SymtabVisitor and DefUseAnalyzeVisitor
+    // then and get the proper DUChains for the variables defined in the variable_block
+    ast::StatementBlock complete_block(functor_block);
+    // Typically variable_block has only one statement, a statement containing the declaration
+    // of the local variables
+    for (const auto& statement: variable_block.get_statements()) {
+        complete_block.insert_statement(complete_block.get_statements().begin(), statement);
+    }


Just as a suggestion: with the similar code, you can also create ast::Program and insert nodes into it. That will solve the problem that we were discussing.

It's a bit pain to do this in the end. Simply adding the StatementBlocks to the Program doesn't work because then SymtabVisitor is crashing due to redefinition of LOCAL variables which are handled as GLOBAL being in the top level of the Program. Instead of this I created a new constructor for the SymtabVisitor to get an empty ModelSymbolTable and be able to process any kind of node.

can't you just clone the variable block? Or are you filtering something out? I do not understand exactly why you do this copy. Clearly is intentional

It's indeed optional. I am creating a new StatementBlock that just includes both the variable and functor_blocks

pramodk · 2021-07-26T16:22:23Z

+    if (modsymtab == nullptr) {
+        modsymtab = new symtab::ModelSymbolTable();
+    }


see the previous comment about avoiding this.

cattabiani · 2021-07-30T08:45:17Z

may we add a small mod file that shows the problem in reduced_dentate? If I check the issue #691 I get a full repo and I do not know where is the problem

iomaganaris · 2021-07-30T09:06:36Z

may we add a small mod file that shows the problem in reduced_dentate? If I check the issue #691 I get a full repo and I do not know where is the problem

This is the mod files I was testing this PR with https://github.com/pramodk/reduced_dentate/blob/master/mechanisms/Aradi_CadepK.mod

cattabiani

I have a few questions and a suggestion for more unit tests. Overall very good job!

cattabiani · 2021-07-30T09:00:47Z

+    // Create complete_block with both variable declarations (done in variable_block) and solver
+    // part (done in functor_block) to be able to run the SymtabVisitor and DefUseAnalyzeVisitor
+    // then and get the proper DUChains for the variables defined in the variable_block
+    ast::StatementBlock complete_block(functor_block);
+    // Typically variable_block has only one statement, a statement containing the declaration
+    // of the local variables
+    for (const auto& statement: variable_block.get_statements()) {
+        complete_block.insert_statement(complete_block.get_statements().begin(), statement);
+    }


can't you just clone the variable block? Or are you filtering something out? I do not understand exactly why you do this copy. Clearly is intentional

cattabiani · 2021-07-30T09:04:35Z

+        if ((variable_type == DUVariableType::Global &&
+             (child_state == DUState::U || child_state == DUState::D)) ||
+            (variable_type == DUVariableType::Local &&
+             (child_state == DUState::LU || child_state == DUState::LD))) {


can we have a DUState::LU or DUState::LD without being a local variable? Otherwise this can be simplified

The discrimination between DUVariableType::Global and DUVariableType::Local is needed so that we can discard any LD or LU of the variable we are analyzing if it's not a local_var on the Symbol Table. I couldn't think of any better way to do this while being as explanatory as possible. It's certainly a quite complex analysis

Just as a note: we discussed different alternatives to avoid introducing new DUVariableType (e.g. by using symbol table current_symtab directly. That avoids new type but we need some way to know the type).

We can go ahead with this implementation and revisit this if we see more use cases requiring further refactoring.

cattabiani · 2021-07-30T09:05:13Z

+        if ((variable_type == DUVariableType::Global && child_state == DUState::U) ||
+            (variable_type == DUVariableType::Local && child_state == DUState::LU)) {


cattabiani · 2021-07-30T09:05:28Z

+        if ((variable_type == DUVariableType::Global && child_state == DUState::D) ||
+            (variable_type == DUVariableType::Local && child_state == DUState::LD) ||
+            child_state == DUState::CD) {


cattabiani · 2021-07-30T09:06:51Z

+/// Variable type processed by DefUseAnalyzeVisitor
+enum class DUVariableType { Local, Global };
+


is this necessary since it could be already specified with LU, LD

EDIT: in this case maybe we could simplify, rework update_defuse_chain to use this new enum. Dunno

Just to clarify, this enum is used to define whether we are looking for the L, U or CD states of variable if it's a global variable in the SymTab or LU, LD or CD if it's a local variable in the SymTab. I should add a comment about this here

cattabiani · 2021-07-30T09:19:20Z

+            REQUIRE(chains[0].eval() == DUState::NONE);
+        }
+    }
+


a few questions:

can you declare also GLOBAL inside the functor?

may we have also the following tests:

2 local variables (used)

2 local variables (1 unused)

2 unused local variables

no local variables

a local variable in an IF ELSE block? Is it possible?

I added a test that includes some of these cases that were not included before. I think it gives a good indication of how the DefUseAnalyzeVisitor works. Let me know if you want to add more cases 👍
GLOBAL variables are defined only in the NEURON block

cattabiani · 2021-07-30T13:27:20Z

https://github.com/pramodk/reduced_dentate/blob/master/mechanisms/Aradi_CadepK.mod

there is not LOCAL inside a functor in this file. I do not connect the dots here. @iomaganaris ?

iomaganaris · 2021-07-30T14:26:26Z

https://github.com/pramodk/reduced_dentate/blob/master/mechanisms/Aradi_CadepK.mod

there is not LOCAL inside a functor in this file. I do not connect the dots here. @iomaganaris ?

The original problem is that in with NMODL master the function operator is const:

void operator()(const Eigen::Matrix<double, 4, 1>& X, Eigen::Matrix<double, 4, 1>& F, Eigen::Matrix<double, 4, 4>& Jm) const {

while it writes variables which are members of the struct functor.
With this PR the qualifier const is not printed, so there is no compilation error with the generated file.

bbpbuildbot · 2021-08-23T17:48:40Z

Logfiles from GitLab pipeline #14671 (:no_entry:) have been uploaded here!

Status and direct links:

bbpbuildbot · 2021-08-23T17:49:02Z

Logfiles from GitLab pipeline #14670 (:no_entry:) have been uploaded here!

Status and direct links:

bbpbuildbot · 2021-08-23T17:57:52Z

Logfiles from GitLab pipeline #14674 (:no_entry:) have been uploaded here!

Status and direct links:

pramodk · 2021-08-24T11:49:26Z

+        if ((variable_type == DUVariableType::Global &&
+             (child_state == DUState::U || child_state == DUState::D)) ||
+            (variable_type == DUVariableType::Local &&
+             (child_state == DUState::LU || child_state == DUState::LD))) {


Just as a note: we discussed different alternatives to avoid introducing new DUVariableType (e.g. by using symbol table current_symtab directly. That avoids new type but we need some way to know the type).

We can go ahead with this implementation and revisit this if we see more use cases requiring further refactoring.

…isitor

bbpbuildbot · 2021-08-24T14:27:10Z

Logfiles from GitLab pipeline #14773 (:no_entry:) have been uploaded here!

Status and direct links:

bbpbuildbot · 2021-08-26T11:27:46Z

Logfiles from GitLab pipeline #14968 (:no_entry:) have been uploaded here!

Status and direct links:

bbpbuildbot · 2021-08-26T14:54:38Z

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

Status and direct links:

iomaganaris · 2021-08-26T15:39:27Z

please retest

pramodk

LGTM

we have discussed some of the comments and certain parts are already addressed.

* Fixes compilation issue when the operator() was declared as const while it changed some of values outside of its scope in the sympy solver * Use DefUseAnalyzeVisitor to find out whether the operator() is Using or Defining any of the variables outside its scope * Update DefUseAnalyzeVisitor to be able to process LOCAL variables as well, given precedence to GLOBAL/RANGE variables, when they have the same name * Added related unit tests * Fix codegen issue in CONSTRUCTOR and DESTRUCTOR blocks code when compiled for CoreNEURON Fixes BlueBrain/nmodl#691 FIxes BlueBrain/nmodl#692 NMODL Repo SHA: BlueBrain/nmodl@1321c7b

iomaganaris added 4 commits June 17, 2021 14:03

Remove const from newton functor definition

fee608d

Add ifdef for coreneuron_build

523d01e

Divide old_* and other variables in the functor of the newton solver

c7171e5

Added documentation about the constructor and destructor issue

e949216

Small fix for dt_saved_value

6f7f7f7

iomaganaris force-pushed the magkanar/fix_newton branch from 42c604b to 6f7f7f7 Compare June 22, 2021 16:42

Merge branch 'master' into magkanar/fix_newton

9aaae4d

iomaganaris marked this pull request as ready for review June 29, 2021 10:54

iomaganaris added 9 commits July 19, 2021 21:28

Trying to use DefUse visitor

39070d7

Segfault

e26d715

Fixed segfault

4d8fa4f

Fixed everything

c7b41f7

Run clang-format

a045000

Fixed tests

0e70c60

Merge branch 'master' into magkanar/fix_newton2

d92e143

Cleared up code a bit

b109f4c

Small cleanup

828ba67

iomaganaris requested a review from pramodk July 23, 2021 14:30

pramodk reviewed Jul 26, 2021

View reviewed changes

iomaganaris added 4 commits July 29, 2021 14:09

Added new SymtabVisitor constructor and unit tests for DefUse

01ef8a9

Used collect_nodes

e1783f9

Make clang-format happy

3c0cefb

Merge branch 'master' into magkanar/fix_newton

4599304

iomaganaris requested a review from cattabiani July 29, 2021 13:45

cattabiani previously requested changes Jul 30, 2021

View reviewed changes

iomaganaris added 5 commits August 23, 2021 18:51

Fix parser unit test with making scenario title smaller

ad5dabb

Added documentation for is_functor_const

2d56466

Added documentation for DUVariableType enum

7724eb5

Added test with multiple simple cases

b0f90ca

Make clang-format happy

a9ab3df

pramodk reviewed Aug 24, 2021

View reviewed changes

iomaganaris added 3 commits August 24, 2021 16:27

Improved checking whether symbol is Global or Local in DefUseAnalyzeV…

64da267

…isitor

Simplify checking of variables in is_functor_const

8aabfd1

Merge branch 'master' into magkanar/fix_newton

63b8002

iomaganaris closed this Aug 26, 2021

iomaganaris reopened this Aug 26, 2021

iomaganaris closed this Aug 26, 2021

iomaganaris reopened this Aug 26, 2021

pramodk approved these changes Aug 27, 2021

View reviewed changes

iomaganaris merged commit 1321c7b into master Aug 27, 2021

iomaganaris deleted the magkanar/fix_newton branch August 27, 2021 08:58

		if ((variable_type == DUVariableType::Global && child_state == DUState::U) \|\|
		(variable_type == DUVariableType::Local && child_state == DUState::LU)) {

		/// Variable type processed by DefUseAnalyzeVisitor
		enum class DUVariableType { Local, Global };

Uh oh!

Conversation

iomaganaris commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbpbuildbot commented Jun 21, 2021

Uh oh!

bbpbuildbot commented Jun 22, 2021

Uh oh!

bbpbuildbot commented Jun 22, 2021

Uh oh!

codecov-commenter commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bbpbuildbot commented Jun 29, 2021

Uh oh!

bbpbuildbot commented Jul 23, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cattabiani commented Jul 30, 2021

Uh oh!

iomaganaris commented Jul 30, 2021

Uh oh!

cattabiani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cattabiani commented Jul 30, 2021

Uh oh!

iomaganaris commented Jul 30, 2021

Uh oh!

bbpbuildbot commented Aug 23, 2021

Uh oh!

bbpbuildbot commented Aug 23, 2021

Uh oh!

bbpbuildbot commented Aug 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbpbuildbot commented Aug 24, 2021

Uh oh!

iomaganaris commented Jun 21, 2021 •

edited

Loading

codecov-commenter commented Jun 29, 2021 •

edited

Loading