Add `CvodeBlock` and `CvodeVisitor` #1467

JCGoran · 2024-09-24T16:07:48Z

Part 1 of #1399.
Adds the original DERIVATIVE block since NMODL solves it in-place. Also adds a visitors which actually replaces statements of the form x' = f(x) into Dx = f(x).

1uc

Two comments:

The DerivativeOriginalBlock is advertised as an unmodified copy. However, it's immediately during the process it's being modified.
There's no code demonstrating how this works, i.e. no tests or anything similar.

1uc · 2024-09-25T07:07:54Z

src/visitors/derivative_original_visitor.hpp

+class DerivativeOriginalVisitor: public AstVisitor {
+  private:
+    /// The copy of the derivative block we are solving
+    ast::DerivativeBlock* der_block_function = nullptr;


Why is this a _function?

Since there's two blocks that will be added: one for Dx = f(x) (the _function one), and the other one for Dx = Dx / (1 - dt * J(f)) (the _jacobian, will be added in next breakdown of the original PR). I've debated about only adding one block, but the codegen would then require parsing the AST and modifying it, which I feel shouldn't be done at the codegen stage, hence the "two blocks, one visitor" design (the one visitor is here only because it felt pointless to make another one which does almost the same thing).

src/visitors/derivative_original_visitor.hpp

src/visitors/derivative_original_visitor.cpp

src/language/codegen.yaml

1uc · 2024-09-25T07:15:00Z

src/language/codegen.yaml

@@ -87,7 +87,30 @@
                                  type: StatementBlock
                              - finalize_block:
                                  brief: "Statement block to be executed after calling linear solver"
-                                  type: StatementBlock                                  
+                                  type: StatementBlock
+                        - DerivativeOriginalFunctionBlock:


Why the Function in DerivativeOriginalFunctionBlock?

NMODL doesn't allow two blocks with the same type (doesn't matter that they may not have the same name), and we will need another block for the transformation involving the Jacobian, so it's basically with that in mind that there's the additional Function. Open to suggestions on how to make it shorter though!

`DERIVATIVE` blocks can't have array variables in NOCMODL by default, so let's go with that.

1uc · 2024-09-30T07:26:02Z

src/visitors/derivative_original_visitor.hpp

+ */
+
+/**
+ * \class DerivativeOriginalVisitor


There's still a couple of places where the old term DerivativeOriginal exists (including filenames).

1uc · 2024-09-30T07:27:20Z

src/visitors/derivative_original_visitor.hpp

+class CvodeVisitor: public AstVisitor {
+  private:
+    /// The copy of the derivative block we are solving
+    std::shared_ptr<ast::DerivativeBlock> der_block = nullptr;


The abbreviation is slightly jarring.

1uc · 2024-09-30T07:29:03Z

src/visitors/derivative_original_visitor.hpp

+    std::shared_ptr<ast::DerivativeBlock> der_block = nullptr;
+
+    /// true while visiting differential equation
+    bool differential_equation = false;


Are the names for differential_equation and derivative_block idiomatic? Could they have a better name? Something the hints better at what they are: inside_*, beneath_*, is_*, printing_*

1uc · 2024-09-30T07:30:26Z

test/unit/visitor/derivative_original.cpp

+        auto ast = run_cvode_visitor(nmodl_text);
+        THEN("CVODE block is added") {
+            auto block = collect_nodes(*ast, {ast::AstNodeType::CVODE_BLOCK});
+            REQUIRE(!block.empty());


It has too be exactly one I think. At least it looks like that's the assumption further down.

1uc · 2024-09-30T07:35:11Z

test/unit/visitor/derivative_original.cpp

+)";
+        auto ast = run_cvode_visitor(nmodl_text);
+        THEN("CVODE block is added") {
+            auto block = collect_nodes(*ast, {ast::AstNodeType::CVODE_BLOCK});


It's a vector of all CVODE_BLOCKs. Hence plural, blocks (or cvode_blocks); then later you'd be free to use block = block[0] (not much use).

1uc · 2024-09-30T07:38:55Z

src/visitors/derivative_original_visitor.cpp

+    }
+
+    // re-visit the AST since we now inserted the DERIVATIVE_ORIGINAL block
+    node.visit_children(*this);


Why do we need to revisit them again?

1uc · 2024-09-30T07:45:42Z

src/visitors/derivative_original_visitor.cpp

+    program_symtab = node.get_symbol_table();
+    node.visit_children(*this);
+    if (der_block) {
+        auto der_node = new ast::CvodeBlock(der_block->get_name(),
+                                            der_block->get_statement_block(),
+                                            der_block->get_statement_block());
+        node.emplace_back_node(der_node);
+    }


If you don't like the pattern that you need to toggle two booleans there's a way out. The code I marked essentially does a:

auto blocks = collect_nodes(*ast, {ast::AstNodeType::DERIVATIVE_BLOCK});

Then all you need to do is run the {}' -> d{} replacement visitor (which consists of the visit_binary_expression) on each block and put them into CVodeBlocks and add them to the program.

Then there's no more boolean toggling; and it'll be more explicit about handling multiple DERIVATIVE blocks. BTW, the name for toggling being inside a cvode block is now rather misleading.

Uses finite differences

Add DerivativeOriginalFunctionBlock and DerivativeVisitor

417acac

JCGoran changed the title ~~Add DerivativeOriginalFunctionBlock and DerivativeVisitor~~ Add DerivativeOriginalFunctionBlock and DerivativeOriginalVisitor Sep 24, 2024

Remove unused functions

d33a594

bbpadministrator pushed a commit to BlueBrain/nmodl-references that referenced this pull request Sep 24, 2024

References for 'BlueBrain/nmodl#1467'.

4bcb324

JCGoran marked this pull request as ready for review September 24, 2024 18:04

1uc reviewed Sep 25, 2024

View reviewed changes

JCGoran added 3 commits September 25, 2024 09:27

Add test for DerivativeOriginalVisitor

b9f08d0

Fmt

c5dc45e

Fix leak

1dadd7a

bbpadministrator pushed a commit to BlueBrain/nmodl-references that referenced this pull request Sep 25, 2024

References for 'BlueBrain/nmodl#1467'.

0cd60c8

JCGoran added 2 commits September 25, 2024 11:32

Remove unused stuff

1125fdf

`DERIVATIVE` blocks can't have array variables in NOCMODL by default, so let's go with that.

Update block description

0267fbd

bbpadministrator pushed a commit to BlueBrain/nmodl-references that referenced this pull request Sep 25, 2024

References for 'BlueBrain/nmodl#1467'.

2e2abdb

JCGoran mentioned this pull request Sep 25, 2024

Add DerivativeOriginalJacobianBlock #1471

Draft

Rename DERIVATIVE_ORIGINAL to CVODE

e58070f

JCGoran changed the title ~~Add DerivativeOriginalFunctionBlock and DerivativeOriginalVisitor~~ Add CvodeBlock and CvodeVisitor Sep 27, 2024

bbpadministrator pushed a commit to BlueBrain/nmodl-references that referenced this pull request Sep 27, 2024

References for 'BlueBrain/nmodl#1467'.

830a0e4

Finish renaming

044dfd9

1uc reviewed Sep 30, 2024

View reviewed changes

JCGoran marked this pull request as draft September 30, 2024 08:08

JCGoran added 10 commits September 30, 2024 12:06

Add item with Jacobian

50f38ce

Merge branch 'master' into jelic/cvode_visitors

bc68701

Do not use an int but an enum-wrapped int

f82fe1f

Add support for diffing expressions with indexed vars

bd2fd36

Allow diffing implicit functions in differentiate2c

b082f0d

Uses finite differences

Simplify condition

edf33a7

Better testing

565fa03

Add suggestions from code review

6bd6aed

Add stepsize param to differentiate2c

0eba407

Merge remote-tracking branch 'origin/master' into jelic/diff_implicit

405909e

JCGoran added 3 commits October 2, 2024 12:45

Try Python 3.9 maybe?

c1e7fd3

Merge branch 'jelic/diff_indexed' into jelic/cvode_visitors

0207373

Merge branch 'jelic/diff_implicit' into jelic/cvode_visitors

32d36a8

JCGoran mentioned this pull request Oct 2, 2024

Codegen for CVODE #1493

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `CvodeBlock` and `CvodeVisitor` #1467

Add `CvodeBlock` and `CvodeVisitor` #1467

JCGoran commented Sep 24, 2024

1uc left a comment

1uc Sep 25, 2024

JCGoran Sep 25, 2024

1uc Sep 25, 2024

JCGoran Sep 25, 2024

1uc Sep 30, 2024

1uc Sep 30, 2024

1uc Sep 30, 2024

1uc Sep 30, 2024

1uc Sep 30, 2024

1uc Sep 30, 2024

1uc Sep 30, 2024

Add CvodeBlock and CvodeVisitor #1467

Are you sure you want to change the base?

Add CvodeBlock and CvodeVisitor #1467

Conversation

JCGoran commented Sep 24, 2024

1uc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `CvodeBlock` and `CvodeVisitor` #1467

Add `CvodeBlock` and `CvodeVisitor` #1467