search

LLNL / sundials

Official development repository for SUNDIALS - a SUite of Nonlinear and DIfferential/ALgebraic equation Solvers. Pull requests are welcome for bug fixes and minor changes.
https://computing.llnl.gov/projects/sundials
BSD 3-Clause "New" or "Revised" License
523 stars 131 forks source link

Maintenance/arkode UI #459

Closed drreynolds closed 7 months ago

drreynolds commented 7 months ago

This is a draft PR, that includes only the code changes to ARKODE to create a shared UI for all steppers. I'm opening this now, in its current form, so that the team can provide feedback.

Remaining items before this is a "final" PR:

  1. Reorganize the ordering of functions in the header and implementation files, to group them according to UI vs internal, and to separate the now-deprecated functions. I'm leaving them in their current locations to make the GitHub "diff" more usable.
drreynolds commented 7 months ago

Overall, I this looks great and it will make creating a stepper significantly easier. I think most or all of my requests are very minor changes.

Thanks for all of the suggestions where I forgot to include ark_mem when calling arkProcessError

I do have a little bit of concern that users will find all of the deprecation messages they are about to get very annoying (if it breaks their builds due to strict treatment of warnings for example), but I suppose there is not really a good way of avoiding that unless we want to support the old functions indefinitely.

I think it will be fine; if it takes a cluttered build log to convince users to eventually update their codes, then so be it. Another good reason to explicitly mark them as deprecated now is that it's a very clear indicator of which pieces should not be included when creating new ARKODE steppers.

I have not made it through every file (in particular the examples), but I looked at all of the main pieces.

drreynolds commented 7 months ago

Revised plan:

  1. Once everyone is happy with this PR as-is, we can go ahead and merge.
  2. In a very-easy-to-review subsequent PR, I'll adjust the order of the contents in the source files to group them more logically into "deprecated", "user-facing", "internal", and "stepper routines provided to ARKODE" sections.