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

Add time offset for LowStorageIntegrator #1151

Merged
merged 12 commits into from
Aug 26, 2024
Merged

Conversation

pbrady
Copy link
Collaborator

@pbrady pbrady commented Aug 9, 2024

PR Summary

I was wanting to supply a time dependent source term (for manufactured solutions) using the LowStorageIntegrator and didn't see a direct way to access the time level associated with each stage. I followed the procedure in Ketcheson 2010 to compute the Butcher table coefficients for the methods. The resulting cs were tested out in a Mathematica notebook and recovered the expected order of accuracy.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Nice improvement!

@Yurlungur Yurlungur enabled auto-merge August 11, 2024 19:06
@pbrady
Copy link
Collaborator Author

pbrady commented Aug 12, 2024

Seems like a CI failure:

Error: Could not find file '/home/runner/work/_actions/_temp_68ccd911-c253-4309-82e1-094e899b9ceb/_staging/gregsdennis-dependencies-action-f4fa194/node_modules/.bin/esparse'.

@pgrete
Copy link
Collaborator

pgrete commented Aug 13, 2024

Seems like a CI failure:

Error: Could not find file '/home/runner/work/_actions/_temp_68ccd911-c253-4309-82e1-094e899b9ceb/_staging/gregsdennis-dependencies-action-f4fa194/node_modules/.bin/esparse'.

Yes, somehow the automated docker image cleanup didn't work as expected. I'm looking into it.

@pgrete
Copy link
Collaborator

pgrete commented Aug 14, 2024

Seems like a CI failure:

Error: Could not find file '/home/runner/work/_actions/_temp_68ccd911-c253-4309-82e1-094e899b9ceb/_staging/gregsdennis-dependencies-action-f4fa194/node_modules/.bin/esparse'.

Yes, somehow the automated docker image cleanup didn't work as expected. I'm looking into it.

Things should work as before again.

Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

Thanks for the addition.
Would you mind adding to the doc (https://parthenon-hpc-lab.github.io/parthenon/develop/src/integrators.html) that c is now also available for the low storage integrators (and potentially even the conversion formula you used to calculate c)?
Also a changelog entry would be great.


delta[2] = 0.0;
beta[2] = 0.016239790859612;
gam0[2] = 0.035802535958088;
gam1[2] = 0.964197464041912;
c[2] = 1.0537621812245777;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is a time offset >1 allowed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is unexpected. I observed the expected order of convergence so I assumed it was correct but am not an expert in SSP RK methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't find any restrictions c so I experimented with changing c[2] <- 1.0 and the result was a loss of accuracy (to 1st order).

@pbrady
Copy link
Collaborator Author

pbrady commented Aug 14, 2024

I've updated the docs and changelog. Computing c is more of an algorithm than a formula so I referenced the paper instead of repeating the algorithm.

@Yurlungur
Copy link
Collaborator

CI failing. @pbrady will plan to add mathematica notebook.

@Yurlungur Yurlungur disabled auto-merge August 22, 2024 16:45
@Yurlungur Yurlungur enabled auto-merge August 22, 2024 16:45
@pbrady
Copy link
Collaborator Author

pbrady commented Aug 23, 2024

Mathematica notebook added

@pgrete
Copy link
Collaborator

pgrete commented Aug 26, 2024

I removed the binary parts (mostly the rendered images) from the notebook file as they don't render on Github anyway (and added one sentence pointing to the notebook in both the docs in head file). Any objections?

@pbrady
Copy link
Collaborator Author

pbrady commented Aug 26, 2024

No objections here.

@pgrete pgrete disabled auto-merge August 26, 2024 14:26
@pgrete pgrete enabled auto-merge (squash) August 26, 2024 14:26
@pgrete pgrete merged commit e0bd43b into develop Aug 26, 2024
53 checks passed
@pbrady pbrady deleted the ptb/add-low-storage-time branch August 28, 2024 16:10
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.

3 participants