Skip to content

Feature: ARKodeGetNumRhsEvals#587

Merged
gardner48 merged 38 commits intodevelopfrom
feature/arkodegetnumrhsevals
Oct 15, 2024
Merged

Feature: ARKodeGetNumRhsEvals#587
gardner48 merged 38 commits intodevelopfrom
feature/arkodegetnumrhsevals

Conversation

@gardner48
Copy link
Copy Markdown
Member

Add an ARKODE level function to get the number of RHS evaluations

@gardner48
Copy link
Copy Markdown
Member Author

gardner48 commented Oct 3, 2024

Related discussions in PR #541, here and here

Copy link
Copy Markdown
Collaborator

@drreynolds drreynolds left a comment

Choose a reason for hiding this comment

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

This is great. I vote that we get this finalized, approved, and merged pronto, so that the LSRKStep PR #541 and the operator-splitting PR #572 can be updated accordingly.

Comment thread CHANGELOG.md Outdated
Comment thread doc/arkode/guide/source/Usage/User_callable.rst Outdated
Comment thread doc/shared/RecentChanges.rst Outdated
drreynolds
drreynolds previously approved these changes Oct 3, 2024
Copy link
Copy Markdown
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.

One minor change request, otherwise this looks great.

Comment thread src/arkode/arkode_arkstep_io.c Outdated
.. versionadded:: 6.1.0


.. c:function:: int ARKodeGetNumRhsEvals(void* arkode_mem, int num_rhs, long int* num_rhs_evals)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

While this unifies all the RHS eval getter, the num_rhs argument is still closely tied to the type of stepper. Each stepper already "knows" it's number of RHS' and how many values to write to num_rhs_evals, so in that sense the argument is not needed. Unfortunately, it is needed for validation but still doesn't guarantee num_rhs_evals has sufficient storage.

I'd like to pitch what I think is a somewhat cleaner option:

int ARKodeGetNumRhsEvals(void* arkode_mem, int partition_idx, long int* num_rhs_evals)

This writes a single number to num_rhs_evals corresponding to the evals for partition partition_idx. If partition_idx<0 that can be the total across all RHS'. This is essentially what I did for counting evolves in operator splitting:

https://github.com/LLNL/sundials/blob/7cd34e8720423a61b0f7c88edcb61d7ef4be075e/doc/arkode/guide/source/Usage/SplittingStep/User_callable.rst?plain=1#L127-L148

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think this sounds good and is a bit cleaner. @drreynolds what do you think?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm good with either approach.

balos1
balos1 previously approved these changes Oct 3, 2024
Steven-Roberts
Steven-Roberts previously approved these changes Oct 3, 2024
Copy link
Copy Markdown
Collaborator

@Steven-Roberts Steven-Roberts left a comment

Choose a reason for hiding this comment

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

One minor comment but otherwise looks good.

Comment thread doc/arkode/guide/source/Usage/User_callable.rst Outdated
gardner48 added a commit to sundials-codes/answers that referenced this pull request Oct 4, 2024
@balos1 balos1 added this to the SUNDIALS Next milestone Oct 8, 2024
@drreynolds drreynolds mentioned this pull request Oct 10, 2024
@gardner48 gardner48 merged commit 554fa92 into develop Oct 15, 2024
@gardner48 gardner48 deleted the feature/arkodegetnumrhsevals branch October 15, 2024 19:58
gardner48 added a commit that referenced this pull request Jul 21, 2025
Add an ARKODE level function to get the number of RHS evaluations

---------

Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu>
Co-authored-by: Steven Roberts <roberts115@llnl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants