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

Feature/sunadjoint #559

Open
wants to merge 183 commits into
base: feature/sunstepper
Choose a base branch
from
Open

Feature/sunadjoint #559

wants to merge 183 commits into from

Conversation

balos1
Copy link
Member

@balos1 balos1 commented Aug 27, 2024

No description provided.

Copy link
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.

Since some commits are being made while reviewing, I'll just submit what I have now so they don't get out of sync. I'm mostly through the docs

.clangd Outdated Show resolved Hide resolved
doc/arkode/guide/source/Mathematics.rst Outdated Show resolved Hide resolved
doc/arkode/guide/source/Mathematics.rst Outdated Show resolved Hide resolved
doc/arkode/guide/source/Mathematics.rst Outdated Show resolved Hide resolved
doc/arkode/guide/source/Mathematics.rst Outdated Show resolved Hide resolved
.. c:function:: SUNErrCode SUNAdjointStepper_RecomputeFwd(SUNAdjointStepper adj_stepper, int64_t start_idx,\
sunrealtype t0, sunrealtype tf, N_Vector y0)

Evolves the forward system in time from start_idx/t0 to stop_idx/tf with dense checkpointing.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unclear if start_idx/t0 is a division?

doc/shared/sunadjoint/SUNAdjointStepper.rst Outdated Show resolved Hide resolved
doc/shared/sundials.bib Outdated Show resolved Hide resolved
doc/shared/sundials.bib Outdated Show resolved Hide resolved
doc/shared/sundials.bib Outdated Show resolved Hide resolved
Copy link
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.

These are the comments for the rest of the docs

doc/shared/sunadjoint/SUNAdjointCheckpointScheme.rst Outdated Show resolved Hide resolved
doc/shared/sunadjoint/SUNAdjointCheckpointScheme.rst Outdated Show resolved Hide resolved
doc/shared/sunadjoint/SUNAdjointCheckpointScheme.rst Outdated Show resolved Hide resolved
doc/shared/sunadjoint/SUNAdjointCheckpointScheme.rst Outdated Show resolved Hide resolved
doc/shared/sunadjoint/SUNAdjointCheckpointScheme.rst Outdated Show resolved Hide resolved
doc/shared/sunadjoint/SUNAdjointCheckpointScheme.rst Outdated Show resolved Hide resolved
doc/shared/sunadjoint/SUNAdjointCheckpointScheme.rst Outdated Show resolved Hide resolved
doc/shared/sunadjoint/SUNAdjointCheckpointScheme.rst Outdated Show resolved Hide resolved
Comment on lines +188 to +191
Allocates a ``SUNMemory`` object whose ``ptr`` field is allocated for
``mem_size`` bytes and is of type ``mem_type``. The new object will have
ownership of ``ptr`` and will be deallocated when
:c:func:`SUNMemoryHelper_Dealloc` is called.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be copied from SUNMemoryHelper_Alloc, and it's unclear to me if the description needs to be different to account for the stride argument.

The implementation provides the following operations defined by the
``SUNMemoryHelper`` API:

.. c:function:: SUNErrCode SUNMemoryHelper_Alloc_Sys(SUNMemoryHelper helper, \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize this is following the convention of the other SUNMemoryHelpers so this will need to be considered outside this PR, but I don't see why these should be made public. Shouldn't users call SUNMemoryHelper_Alloc rather than SUNMemoryHelper_Alloc_Sys?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gardner48 Do you think we should document the implementation specific overrides of the generic functions? We do so somewhat inconsistently as of now .

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