Fix symbolic bug #252
Fix symbolic bug #252
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
- Coverage 48.71% 48.56% -0.15%
==========================================
Files 31 31
Lines 8313 8351 +38
==========================================
+ Hits 4050 4056 +6
- Misses 4263 4295 +32 ☔ View full report in Codecov by Sentry. |
mjohnson541
force-pushed
the
fix_symbolic_bug
branch
from
0c32665
to
de231cf
Compare
|
I've also added finite differences as a Jacobian source and improved our handling of jac_prototype. |
mjohnson541
force-pushed
the
fix_symbolic_bug
branch
from
1e1f7d5
to
28e9b36
Compare
mjohnson541
force-pushed
the
fix_symbolic_bug
branch
from
5533751
to
f422946
Compare
|
Decided to not add finite differences because how much the accuracy of the Jacobian degrades. |
src/PhaseState.jl
Outdated
| if signbit(f) | ||
| kf = getkf(rxn,ph,T,P,C,ns,V,phi) | ||
| else | ||
| if isa(f,Num) || !signbit(f) |
There was a problem hiding this comment.
Can you run a formatting script over your changes or this file?
There was a problem hiding this comment.
I'm having difficulty getting the formatter extension to connect with the julia kernel. Was there anything in particular you had in mind?
There was a problem hiding this comment.
Could you add blank after comma?
There was a problem hiding this comment.
I ran formatter on the files changed in this PR, so I'm good with the formatting
| @@ -310,7 +327,7 @@ function Reactor(domain::T, y0unlumped::Array{W1,1}, tspan::Tuple, reducedmodelm | |||
| if modelingtoolkit | |||
| sys = modelingtoolkitize(ode) | |||
| jac = eval(ModelingToolkit.generate_jacobian(sys)[2]) | |||
| odefcn = ODEFunction(dydt; jac=jac, paramjac=jacp!) | |||
| odefcn = ODEFunction(dydt; jac=jac, paramjac=jacp!, jac_prototype=jac_prototype) | |||
There was a problem hiding this comment.
Is modelingtoolkitize tested in our test? Maybe we should add a test
There was a problem hiding this comment.
We should, but we need a very small mechanism to make it fast.
Co-authored-by: Hao-Wei Pang <45482070+hwpang@users.noreply.github.com>
.github/workflows/Install.yml
Outdated
| - uses: actions/checkout@v2 | ||
| - uses: julia-actions/setup-julia@v1 | ||
| with: | ||
| version: 1.8 |
There was a problem hiding this comment.
Why do we pin to 1.8? What is preventing us to move up to 1.10?
Fix bug when feeding symbolic variables through getkfkrev.