Make "sparse" solver check if equations are linear. #860
Conversation
Codecov Report
@@ Coverage Diff @@
## master #860 +/- ##
==========================================
- Coverage 61.41% 61.40% -0.02%
==========================================
Files 208 208
Lines 30048 30081 +33
==========================================
+ Hits 18455 18472 +17
- Misses 11593 11609 +16
Continue to review full report at Codecov.
|
If the system is linear, then newtons method always converges in exactly one iteration. When using the sparse solver on linear systems omit the newtons iteration and solve directly. This should make the resulting code run marginally faster by skipping the check for convergence. Currently the check for convergence is implemented as "error = sqrt(|F|^2)".
ctrl-z-9000-times
force-pushed
the
sparse-linear
branch
from
d868bd3
to
00a0296
Compare
| nmodl_eigen_f[0] = 5.0-nmodl_eigen_x[0] | ||
| nmodl_eigen_j[0] = -1.0 | ||
| nmodl_eigen_f[0] = 5.0-pow(nmodl_eigen_x[0], 3) | ||
| nmodl_eigen_j[0] = -3.0 * pow(nmodl_eigen_x[0], 2) |
There was a problem hiding this comment.
I will wait for comments from @cattabiani but just from code generation perspective I want to point out that use of pow is expensive and typically we want to rewrite/simplify x^2 to x*x. IIRC, we use somewhere sympy to do this simplification.
There was a problem hiding this comment.
ok, sorry, I missread the comment. IIRC pow was used because we are not sure, in general, that the exponent is a positive integer
There was a problem hiding this comment.
a few points about replacing pows:
- it brings some marginal improvements (I believe) if it remains robust and sound for all the mod files. It could be more error-prone from the user perspective. I let @alkino verify this
- pow has always being used, since I stated working on nmodl. I did not know about this Liam adversity to the function. I do not think it is really related to this pr
- the way to change it (that I see atm) would be to partially remove pow from the list of available functions in ode.py or a replacement afterward. Both ways look very cumbersome, hacky and maybe error-prone for not a lot of improvement
There was a problem hiding this comment.
The reason "pow" appears in this PR is because I changed the testcase from solving a linear equation "x=5" to solving a non-linear equation "x^3=5".
I changed that test because it is a test for the nonlinear solver, and I wanted it to continue testing the same code paths.
There was a problem hiding this comment.
Adding info from @cattabiani via Slack:
the relevant ticket and mod file that needs to be be checked with this PR is: #801
There was a problem hiding this comment.
Ok, I ran this PR on the file cdp5.mod and it works.
It successfully runs and the resulting C++ file correctly uses newtons method.
Here is the file in question:
https://senselab.med.yale.edu/ModelDB/showmodel?model=239421&file=/Purkinjecell_2017/mod_files/cdp5.mod#tabs-2
If the system is linear, then newtons method always converges
in exactly one iteration. When using the sparse solver on
linear systems omit the newtons iteration and solve directly.
This should make the resulting code run marginally faster by
skipping the check for convergence. Currently the check for
convergence is implemented as "error = sqrt(|F|^2)".