Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix calculate_derivatives for PETSc #1342
Fix calculate_derivatives for PETSc #1342
Changes from 2 commits
3370f4a
731783a
6e9148a
432921e
e5c8a29
8498287
2a74982
cbd10a1
abfaf11
9df11af
4b589ce
4ad0e2b
6decdda
c904006
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a quick look I'd guess it might be because of
old_value
is defined inside theif
block on L764?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it outside the
if
block, still getting the pylint error. I'm guessing it's because Pylint doesn't know that accessingderiv[t].value
is only going to raise aKeyError
in that context, and will never raise aValueError
. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it might work if
old_value = deriv[t].value
is placed at the top of thefor t in time:
loop block, i.e. on line 756 (before theif t < between.first() ...
line)?If that doesn't work either, I agree that a Pylint
disable
directive seems warranted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 therepresentative_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried deactivating
m.diffeq[2]
, and it still gets populated. It isn't populated by thecalculate_derivatives
function, but directly from the solver.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.