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.

Sign up for GitHub

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

Jump to bottom

Feature/arkode sts #541

Open

maggul wants to merge 349 commits into develop from feature/arkode-sts

+11,817 −310

Collaborator

maggul commented Jul 12, 2024

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

maggul added the pkg-ARKODE label

Collaborator

drreynolds commented Jul 12, 2024

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.

drreynolds force-pushed the feature/arkode-sts branch from 12940d0 to 62bccbd Compare

July 12, 2024 16:14

Member

balos1 commented Jul 12, 2024

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.

Collaborator

drreynolds commented Jul 12, 2024

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.

drreynolds reviewed

View reviewed changes

include/arkode/arkode_lsrkstep.h Outdated Show resolved Hide resolved

drreynolds reviewed

View reviewed changes

include/arkode/arkode_lsrkstep.h Outdated Show resolved Hide resolved

drreynolds reviewed

View reviewed changes

include/arkode/arkode_lsrkstep.h Outdated Show resolved Hide resolved

drreynolds reviewed

View reviewed changes

include/arkode/arkode_lsrkstep.h Outdated Show resolved Hide resolved

drreynolds reviewed

View reviewed changes

include/arkode/arkode_lsrkstep.h Outdated Show resolved Hide resolved

drreynolds reviewed

View reviewed changes

include/arkode/arkode_lsrkstep.h Show resolved Hide resolved

drreynolds reviewed

View reviewed changes

include/arkode/arkode_lsrkstep.h Outdated Show resolved Hide resolved

drreynolds reviewed

View reviewed changes

examples/arkode/C_serial/CMakeLists.txt Outdated Show resolved Hide resolved

drreynolds reviewed

View reviewed changes

Collaborator

drreynolds left a comment

I still need to look at the changes in src/arkode, but I wanted to share my comments so far.

examples/arkode/C_serial/erk_analytic.c Outdated Show resolved Hide resolved

examples/arkode/C_serial/erk_analytic.out Outdated Show resolved Hide resolved

examples/arkode/C_serial/ark_analytic.c Outdated Show resolved Hide resolved

examples/arkode/C_serial/lsrk_analytic.c Outdated Show resolved Hide resolved

examples/arkode/C_serial/lsrk_analytic.c Outdated Show resolved Hide resolved

examples/arkode/C_serial/lsrk_analytic.c Outdated Show resolved Hide resolved

examples/arkode/C_serial/lsrk_analytic_VarJac.c Outdated Show resolved Hide resolved

examples/arkode/C_serial/lsrk_analytic_VarJac.c Outdated Show resolved Hide resolved

examples/arkode/C_serial/lsrk_analytic_VarJac.c Outdated Show resolved Hide resolved

examples/arkode/C_serial/lsrk_analytic_VarJac.c Outdated Show resolved Hide resolved

drreynolds reviewed

View reviewed changes

Collaborator

drreynolds left a comment

One more set of comments, covering the rest of this draft PR.

src/arkode/arkode_lsrkstep_impl.h Outdated Show resolved Hide resolved

src/arkode/arkode_lsrkstep_impl.h Outdated Show resolved Hide resolved

src/arkode/arkode_lsrkstep_impl.h Outdated Show resolved Hide resolved

src/arkode/arkode_lsrkstep_impl.h Outdated Show resolved Hide resolved

src/arkode/arkode_lsrkstep_io.c Outdated Show resolved Hide resolved

src/arkode/arkode_lsrkstep.c Outdated Show resolved Hide resolved

src/arkode/arkode_lsrkstep.c Outdated Show resolved Hide resolved

src/arkode/arkode_lsrkstep.c Outdated Show resolved Hide resolved

src/arkode/arkode_lsrkstep.c Outdated Show resolved Hide resolved

src/arkode/arkode_lsrkstep.c Outdated Show resolved Hide resolved

drreynolds and others added 15 commits

July 31, 2024 08:39


          Updated ark_heat2D.cpp with changes from develop branch (mainly fixes…

f1e8be9

… to comments)


          Removed commented-out lines

672117f


          Removed hard-coded print statement, since it is replicated on all MPI…

8b8a1f5

… ranks


          Minor cleanup

c153a68


          Added MPI-parallel arkHeat2D example that uses LSRKStep

6a43a06


          Added better error handling to ark_heat2D_hypre_pfmg.cpp

69d5d1c


          Added explicit support for diffusion-2D benchmark (using LSRKStep)

73b41be


          Some progress - SSP

b188ace


          SSPs2 is ready!

ad52047


          SSPs3 is ready!

ff85f5d


          SSP104 is ready!

a94a3e2


          LSRKStepSetSSPStageNum

9fd9d81


          allow s=10 in LSRKStepSetSSPStageNum for SSP10_4

0dc3db6


          Documentation

60a9fdc


          SSP Stats

fc81648

maggul and others added 5 commits

October 9, 2024 22:14


          update output files

2f2fc23


          Formatting

ddf677b


          update output files

67e2272


          update output files


          Fixed specification of Lagrange interpolation default

52766a9

Collaborator

drreynolds commented Oct 10, 2024

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.

drreynolds requested review from drreynolds, balos1, Steven-Roberts and gardner48

October 10, 2024 13:34

drreynolds added 4 commits

October 10, 2024 11:55


          Updated mechanism for setting up default SSP and STS methods

eb06cd2


          Merge branch 'feature/arkode-sts' of github.com:LLNL/sundials into fe…

bb6b15a

…ature/arkode-sts


          formatting

9c2eb5c


          A few more minor cleanup items

e9f30fc

drreynolds previously approved these changes

View reviewed changes

drreynolds dismissed stale reviews from balos1, Steven-Roberts, and gardner48

October 10, 2024 17:47

All comments appear to be resolved, so I'm dismissing this stale review.

drreynolds and others added 3 commits

October 15, 2024 13:35


          Merge branch 'develop' into feature/arkode-sts

dc090e9


          merge with develop

1e3bafd


          GetNumRhsEvals

65b2ddd

maggul dismissed drreynolds’s stale review via

65b2ddd

October 16, 2024 00:48

maggul added 4 commits

October 15, 2024 19:53


          Formatting and swig

62cb9ec


          update outputs

cc81dfd


          functions reordering

8ddc778


          formatting

4e89b8e

drreynolds requested changes

View reviewed changes

Collaborator

drreynolds left a comment

One minor error, but otherwise I think this looks good.

doc/arkode/guide/source/Usage/LSRKStep/User_callable.rst Outdated Show resolved Hide resolved


          documentation

5e530ec

drreynolds approved these changes

View reviewed changes

balos1 reviewed

View reviewed changes

src/arkode/arkode.c

                 ARKodeHAdaptMem hadapt_mem;
                 if (*nflagPtr == ARK_SUCCESS) { return (ARK_SUCCESS); }
+                /* Returns with an ARK_RETRY_STEP flag happen in a step much

Member

balos1 Oct 18, 2024 •

edited

Loading

I do not understand what this comment is saying. Can you clarify?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

balos1 balos1 left review comments

drreynolds drreynolds approved these changes

cswoodward Awaiting requested review from cswoodward cswoodward is a code owner

Steven-Roberts Awaiting requested review from Steven-Roberts

gardner48 Awaiting requested review from gardner48 gardner48 is a code owner

Labels