-
Notifications
You must be signed in to change notification settings - Fork 2
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
Dolci/schedules application #56
Conversation
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.
Some general comments:
- I think there is an error in the forward residual -- the boundary conditions don't seem to be applied.
- The adjoint derivation is motivated by a continuous adjoint approach, but the later Taylor testing verifies discrete adjoint consistency.
- The displayed adjoint sensitivity is a directional derivative rather than the full gradient.
- I recommend reducing the number of example schedules.
- I recommend removing all the code comments in the example schedules.
- The disk storage schedules don't actually store checkpoints on disk.
- I don't understand the
single_storage
behaviour.
… required in the reviews
Co-authored-by: James R. Maddison <[email protected]>
Co-authored-by: James R. Maddison <[email protected]>
Co-authored-by: James R. Maddison <[email protected]>
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 checking mass and stiffness matrices:
- There is an off-by-on error in the definition of
$h$ , which should bemodel["lx"] / (model["nx"] - 1)
. This appears in three places. - The
_mass_matrix
method returns the mass divided by$h$ , which could be noted in a comment. - The
_stiffness_matrix
method returns the stiffness matrix multiplied by-\model["nu"] / h
. There are sign errors when the result is used in_residual
and_jacobian
.
Co-authored-by: James R. Maddison <[email protected]>
Co-authored-by: James R. Maddison <[email protected]>
Co-authored-by: James R. Maddison <[email protected]>
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.
Code should also be linted (e.g. using ruff).
Co-authored-by: James R. Maddison <[email protected]>
Co-authored-by: James R. Maddison <[email protected]>
No description provided.