Feature/arkode sts

[maggul]

This is a draft PR for feedback on new STS module.

[drreynolds]

Unfortunately, the rebase to develop seems to have marked hundreds of files as conflicts. I'll work on fixing this branch now, so that it will be easier for folks to review.

[balos1]

Unfortunately, the rebase to develop seems to have marked hundreds of files as conflicts. I'll work on fixing this branch now, so that it will be easier for folks to review.

Yeah, I think in the future we should not rebase develop on main (if main is updated directly) and instead do a merge. I don't see a real advantage to having the same linear history for main and develop, and it seems the rebase has a pretty big downside.

[drreynolds]

and it seems the rebase has a pretty big downside.

Indeed. Although rebasing might be no big deal for branches with 1-2 commits, it is a huge waste of time for a branch with any appreciable amount of development.

[gardner48]

It would be good to add a test similar to test/unit_tests/ark_test_dahlquist_erk.cpp that verifies there are no extraneous RHS evaluations

[gardner48]

Ugh! MSVC does not use standard conforming complex types so it's probably best that we keep the separate real and complex outputs. Sorry! I'll revert the suncomplex commit and cherry-pick it over to a new branch so we can use it in the future.

[Steven-Roberts]

This text https://github.com/LLNL/sundials/blob/6117e71c1e49d373b38ef49917740aaae5b55acf/doc/arkode/guide/source/Usage/User_callable.rst?plain=1#L48-L52

and https://github.com/LLNL/sundials/blob/6117e71c1e49d373b38ef49917740aaae5b55acf/doc/arkode/guide/source/Usage/User_callable.rst?plain=1#L60-L61

need to be updated.

This list of steppers is growing long and text like this easily gets overlooked, so I think more generic statements without a long list of links would be ok.

[drreynolds]

I believe that all conversations have now been resolved. The only remaining item to do in the code is to adjust how the number of RHS evaluations is retrieved by the user (addressed in PR #587). Whichever of the two PRs is merged second will need to be updated slightly. I'll request "re-review" by everyone so folks can verify that their concerns have been resolved.

[maggul]

I have completed my revision of the PR. @gardner48, thanks for your comments on the RHS evaluations. I’ve found a way to reduce one more line of vectors, which aligns better with our low-storage claim. This new approach involves recomputing fn if the step fails, which should be infrequent, thereby reducing the extra line of vectors that would otherwise be needed to store fn. Please review the updates and let me know if there’s anything else you need me to address. @drreynolds @balos1 @Steven-Roberts

[drreynolds]

Are the changes in the .gitlab folder intended?

I'm guessing they are not intended. I noticed that after the Gitlab CI PR #595 was merged, my local repository clones showed conflicts that needed to be resolved using

git submodule update --init --recursive

I'm guessing that @maggul may have "resolved" the issue in some other way. @maggul, can you clarify what you did to update feature/arkode-sts with these changes from develop after #595 was merged, since we may need to figure out how to resolve this change correctly?

[balos1]

Are the changes in the .gitlab folder intended?

I'm guessing they are not intended. I noticed that after the Gitlab CI PR #595 was merged, my local repository clones showed conflicts that needed to be resolved using

git submodule update --init --recursive

I'm guessing that @maggul may have "resolved" the issue in some other way. @maggul, can you clarify what you did to update feature/arkode-sts with these changes from develop after #595 was merged, since we may need to figure out how to resolve this change correctly?

@maggul Should just run

git fetch origin
git checkout origin/develop -- .gitlab
git submodule update
git add .gitlab
git commit

and that should fix things.

[Steven-Roberts]

See this diff for a way to avoid potential undefined behavior from round/ceil/floor int conversions. https://github.com/LLNL/sundials/compare/feature/arkode-sts...feature/arkode-sts-test

[maggul]

I’ve completed my part of the revision. Please review my changes and let me know if you notice anything that still needs attention.