Improve docs and code of newmark_ss #267
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Fix equations to match the ones in the code, correct parenthesis, etc
- Bring M^{-1} term to the inside of the matrix multiplying the forces, for consistency of dimensions. Replace curly brackets by square brackets since it is a matrix and not a vector
SHARPy specific notation goes last
This is a numpy best practice when multiplying matrices
Codecov Report
@@ Coverage Diff @@
## develop #267 +/- ##
========================================
Coverage 67.69% 67.70%
========================================
Files 166 166
Lines 26709 26710 +1
========================================
+ Hits 18081 18083 +2
+ Misses 8628 8627 -1
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
sduess
approved these changes
Nov 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes in the documentation of
newmark_ssI spotted some small errors in the documentation of the
newmark_ssfunction (the code was ok!) and I decided to improve them since this is a pretty good tutorial on the method that may be useful for my team as well. Therefore I did some improvements such asChanges in the code of
newmark_ssI tried to make the code slightly faster and more accurate by reusing the factorization of the Ass1 matrix and by using the factorization of the M matrix to compute the multiplication by the inverse (due to the triangular structure of the factorization this is both faster and more accurate). This caused a change in the function arguments since it now takes the M matrix instead of its inverse. I also added the option to factorize the M matrix using Cholesky, but I did not enable that option since I am not familiar enough with SHARPy's theory to be 100% sure that the M matrix is symmetric positive definite. I would like to ask the core developers to consider enabling that option on the calls to
newmark_ss. It has better numerical properties than the LU decomposition. I cleaned up some dead code close to the other changes I made and replaced some of the usages ofnp.dotby@for matrix multiplication, since this is a NumPy best practice.Changes in the installation documentation
I did some small additions to the installation with Docker section that I found useful when I used it for testing:
Testing of the changes
I tested the changes on Docker, and I am pasting the output below. I believe all tests passed but I would like to double check if the warnings are expected.