Fix calculate_derivatives for PETSc #1342
Conversation
dallan-keylogic
requested review from
andrewlee94,
eslickj and
lbianchi-lbl
as code owners
| # TODO I don't understand how a value is getting assigned here | ||
| # assert m.dxdt[2].value is None |
There was a problem hiding this comment.
@eslickj , could you help me understand why we're getting derivative values populated here? It's definitely not through the calculate_derivatives function, but something being returned by the PETSc solver interface.
There was a problem hiding this comment.
I think the problem is that with the forward finite difference discretization you should technically be deactivating m.diffeq[2] however this runs afoul of the representative_time infrastructure in the petsc solver. Seems like there is an assumption baked into the petsc solver logic that the model will be discretized using an implicit approach. Which makes sense since our petsc docs explicitly mention that only "implicit TS solvers are supported".
There was a problem hiding this comment.
I've tried deactivating m.diffeq[2], and it still gets populated. It isn't populated by the calculate_derivatives function, but directly from the solver.
There was a problem hiding this comment.
Wait, no, you're right, it still looks for a differential equation at t=2 and throws an error if it can't find one. I will note that the value returned for dxdt[2] isn't what you'd get with that differential equation, though.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1342 +/- ##
==========================================
- Coverage 77.60% 77.59% -0.01%
==========================================
Files 391 391
Lines 64315 64330 +15
Branches 14235 14244 +9
==========================================
+ Hits 49913 49920 +7
- Misses 11829 11835 +6
- Partials 2573 2575 +2 ☔ View full report in Codecov by Sentry. |
…calculate_derivatives
|
@eslickj, any chance you could review this? |
| # TODO I don't understand how a value is getting assigned here | ||
| # assert m.dxdt[2].value is None |
There was a problem hiding this comment.
I think the problem is that with the forward finite difference discretization you should technically be deactivating m.diffeq[2] however this runs afoul of the representative_time infrastructure in the petsc solver. Seems like there is an assumption baked into the petsc solver logic that the model will be discretized using an implicit approach. Which makes sense since our petsc docs explicitly mention that only "implicit TS solvers are supported".
| if t == between.first() or t == between.last(): | ||
| # Reset deriv value to old value | ||
| if disc_eq[t].active and not deriv[t].fixed: | ||
| deriv[t].value = old_value |
There was a problem hiding this comment.
@lbianchi-lbl Do you know what Pylint is complaining about here? We can only get to this logic if old_value has already been assigned in the try block.
There was a problem hiding this comment.
From a quick look I'd guess it might be because of old_value is defined inside the if block on L764?
There was a problem hiding this comment.
Moved it outside the if block, still getting the pylint error. I'm guessing it's because Pylint doesn't know that accessing deriv[t].value is only going to raise a KeyError in that context, and will never raise a ValueError. I assume this is a case for some Pylint ignore statement.
The test failures appear to be unrelated to this PR.
There was a problem hiding this comment.
I guess it might work if old_value = deriv[t].value is placed at the top of the for t in time: loop block, i.e. on line 756 (before the if t < between.first() ... line)?
If that doesn't work either, I agree that a Pylint disable directive seems warranted.
|
@andrewlee94 @lbianchi-lbl Could you rubber stamp this so integration tests can run? |
|
@dallan-keylogic You can use the |
@eslickj your approval always matters to us! |
Fixes
Addresses #1327
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: