Skip to content
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.

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

Maintenance/arkode UI #459

Merged
merged 44 commits into from
May 7, 2024
Merged

Maintenance/arkode UI #459

merged 44 commits into from
May 7, 2024

Conversation

drreynolds
Copy link
Collaborator

@drreynolds drreynolds commented Apr 18, 2024

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.

Copy link
Member

@balos1 balos1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 have not made it through every file (in particular the examples), but I looked at all of the main pieces.

src/arkode/arkode.c Outdated Show resolved Hide resolved
src/arkode/arkode.c Outdated Show resolved Hide resolved
src/arkode/arkode.c Outdated Show resolved Hide resolved
src/arkode/arkode_arkstep_impl.h Outdated Show resolved Hide resolved
src/arkode/arkode_io.c Outdated Show resolved Hide resolved
src/arkode/arkode_ls.c Outdated Show resolved Hide resolved
src/arkode/arkode_ls.c Outdated Show resolved Hide resolved
src/arkode/arkode_ls.c Outdated Show resolved Hide resolved
src/arkode/arkode_ls.c Outdated Show resolved Hide resolved
src/arkode/arkode_ls.c Outdated Show resolved Hide resolved
Copy link
Member

@gardner48 gardner48 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished a pass over the docs and recent updates to the PR

src/arkode/arkode_io.c Outdated Show resolved Hide resolved
src/arkode/arkode_ls.c Outdated Show resolved Hide resolved
doc/arkode/guide/source/Usage/User_callable.rst Outdated Show resolved Hide resolved
doc/arkode/guide/source/Usage/User_callable.rst Outdated Show resolved Hide resolved
doc/arkode/guide/source/Usage/User_callable.rst Outdated Show resolved Hide resolved
doc/shared/RecentChanges.rst Outdated Show resolved Hide resolved
Copy link
Member

@gardner48 gardner48 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo to fix, otherwise this looks good to me

@drreynolds
Copy link
Collaborator Author

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.

…e.rst

Co-authored-by: David Gardner <gardner48@llnl.gov>
balos1
balos1 previously approved these changes May 6, 2024
test/answers Outdated Show resolved Hide resolved
gardner48 pushed a commit to sundials-codes/answers that referenced this pull request May 6, 2024
@gardner48 gardner48 merged commit 31ffc21 into develop May 7, 2024
20 of 24 checks passed
@gardner48 gardner48 deleted the maintenance/arkode-ui branch May 7, 2024 05:32
gardner48 added a commit that referenced this pull request Jun 20, 2024
Create a shared UI for all ARKODE steppers. 

---------

Co-authored-by: Cody Balos <balos1@llnl.gov>
Co-authored-by: David Gardner <gardner48@llnl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants