Conversation
drreynolds
left a comment
drreynolds
left a comment
There was a problem hiding this comment.
I finished a pass through the revisions. It looks like this is getting close, with a few more minor items to resolve. I added a number of "suggestions" to this review, particularly regarding wording in the documentation.
Co-authored-by: David Gardner <gardner48@llnl.gov>
drreynolds
left a comment
drreynolds
left a comment
There was a problem hiding this comment.
It looks to me like @maggul may have addressed nearly all requests, but multiple of the PR "comments" are not yet marked as resolved. @maggul, please go through each of the as-yet-unresolved comments and either "resolve" them if they have been completed, or add a comment to respond to those that still need discussion/revision.
Done! Please go ahead and rereview it @gardner48 @drreynolds |
drreynolds
left a comment
drreynolds
left a comment
There was a problem hiding this comment.
It looks like @maggul addressed all of the still-visible comments in the PR and marked them as "resolved." However, there were a handful of "outdated" comments that were still unresolved. In going through those, I marked some as "resolved" if it was obvious to me; however, there were three from @gardner48 where I was uncertain. Since GitHub makes it difficult to see "outdated" comments, I re-entered David's comments in the corresponding locations as new comments.
@maggul, please look at these three comments, and either address/resolve or respond.
This PR updates the current SUNDomEigEstimator module to support robust estimation of complex-valued dominant eigenvalues. In addition, it updates the LSRKStep's STS methods to ensure compatibility with systems that have complex dominant eigenvalues. To this end, it enables stability region checks for linear stability analysis and allows adjustment of the number of stages or step sizes to ensure stability for the estimated dominant eigenvalue.