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 support for writing openPMD/ADIOS2 plotfiles #466

Merged
merged 28 commits into from
Dec 13, 2023

Conversation

BenWibking
Copy link
Collaborator

@BenWibking BenWibking commented Dec 8, 2023

Description

This adds support for writing ADIOS2 format plotfiles using openPMD-api (https://github.com/openPMD/openPMD-api). This is the file format used by WarpX for its large-scale runs, and it is the fastest option for large-scale I/O (~Terabytes or larger).

This adds support for cell-centered variables only. (However, it does include cell-centered averages of the face-centered variables, and all of the other derived variables.)

openPMD currently does not support ghost cells, so we exclude them when writing the plotfile in openPMD/ADIOS2 format.

There is no support for checkpoints or restarts with this format (nor is there in WarpX, or any other code that I'm aware of).

Related issues

N/A

Checklist

Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an x inside the square brackets [ ] in the Markdown source below:

  • I have added a description (see above).
  • I have added a link to any related issues see (see above).
  • I have read the Contributing Guide.
  • I have added tests for any new physics that this PR adds to the code.
  • I have tested this PR on my local computer and all tests pass.
  • I have manually triggered the GPU tests with the magic comment /azp run.
  • I have requested a reviewer for this PR.

@BenWibking BenWibking changed the title WIP: add support for writing openPMD/ADIOS plotfiles WIP: add support for writing openPMD/ADIOS2 plotfiles Dec 8, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/openPMD.cpp Outdated Show resolved Hide resolved
src/openPMD.cpp Show resolved Hide resolved
src/openPMD.cpp Outdated Show resolved Hide resolved
src/openPMD.cpp Show resolved Hide resolved
src/openPMD.cpp Show resolved Hide resolved
src/openPMD.cpp Outdated Show resolved Hide resolved
src/openPMD.cpp Show resolved Hide resolved
src/openPMD.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/simulation.hpp Outdated Show resolved Hide resolved
src/simulation.hpp Show resolved Hide resolved
@BenWibking BenWibking added the enhancement New feature or request label Dec 8, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/simulation.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/openPMD.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/openPMD.hpp Show resolved Hide resolved
@BenWibking
Copy link
Collaborator Author

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@BenWibking
Copy link
Collaborator Author

It looks like the clang-tidy complaint about not being able to find the openPMD headers is the last issue to fix.

@BenWibking BenWibking added I/O input/output and removed enhancement New feature or request labels Dec 10, 2023
BenWibking and others added 3 commits December 11, 2023 14:35
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@BenWibking
Copy link
Collaborator Author

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@BenWibking BenWibking changed the title WIP: add support for writing openPMD/ADIOS2 plotfiles add support for writing openPMD/ADIOS2 plotfiles Dec 11, 2023
@BenWibking BenWibking marked this pull request as ready for review December 11, 2023 19:40
Copy link
Collaborator

@markkrumholz markkrumholz left a comment

Choose a reason for hiding this comment

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

  1. It seems like this is only implemented for cell-centered quantities. Did you intend to handle face-centered quantities in a different PR?
  2. I see an implementation of writing, but no implementation of reading. So how does this work with restarts? Or is this used only for plot files, with no implementation for checkpoints?
  3. I have one minor comment on one of the implementations, which is appended to that file.

@BenWibking
Copy link
Collaborator Author

  1. It seems like this is only implemented for cell-centered quantities. Did you intend to handle face-centered quantities in a different PR?
  2. I see an implementation of writing, but no implementation of reading. So how does this work with restarts? Or is this used only for plot files, with no implementation for checkpoints?
  3. I have one minor comment on one of the implementations, which is appended to that file.

Yes, this is only for cell-centered quantities. I will handle face-centered quantities in another PR as needed.

There is no implementation of reading. This is intended only for plotfiles, which have to be written out with the highest frequency. The main goal of this PR is to enable using openPMD-based analysis tools, and get faster plotfile outputs "for free." Also, getting restarts to work with another format would be very error-prone and likely to result in a non-working restart for the first several attempts, so I'd prefer not do that.

(I've replied to your comment in-line.)

@BenWibking
Copy link
Collaborator Author

I've updated the PR description to note the limitations you pointed out above.

@markkrumholz
Copy link
Collaborator

Yes, this is only for cell-centered quantities. I will handle face-centered quantities in another PR as needed.

There is no implementation of reading. This is intended only for plotfiles, which have to be written out with the highest frequency. The main goal of this PR is to enable using openPMD-based analysis tools, and get faster plotfile outputs "for free." Also, getting restarts to work with another format would be very error-prone and likely to result in a non-working restart for the first several attempts, so I'd prefer not do that.

On reading that is fine.

On cc vs fc, that's also fine, but then there should probably be some sort of automation, or at least a warning, that this method is incompatible with fc quantities. For example, you could check if the number of fc quantities is > 0, and issue a warning or throw an error message at compile time if openPMD is on.

@BenWibking
Copy link
Collaborator Author

On cc vs fc, that's also fine, but then there should probably be some sort of automation, or at least a warning, that this method is incompatible with fc quantities. For example, you could check if the number of fc quantities is > 0, and issue a warning or throw an error message at compile time if openPMD is on.

Is outputting the cell-centered averages enough? Otherwise, I could add a warning in this case.

@markkrumholz
Copy link
Collaborator

On cc vs fc, that's also fine, but then there should probably be some sort of automation, or at least a warning, that this method is incompatible with fc quantities. For example, you could check if the number of fc quantities is > 0, and issue a warning or throw an error message at compile time if openPMD is on.

Is outputting the cell-centered averages enough? Otherwise, I could add a warning in this case.

I think it would be good to add a warning, since this means that this output option will behave differently than all the other ones, which do support fc quantities. Differences in behaviour should be flagged.

markkrumholz
markkrumholz previously approved these changes Dec 12, 2023
markkrumholz
markkrumholz previously approved these changes Dec 12, 2023
@BenWibking
Copy link
Collaborator Author

Ok, now it should be right.

@BenWibking BenWibking added this pull request to the merge queue Dec 13, 2023
Merged via the queue into development with commit f7f34fc Dec 13, 2023
11 checks passed
@BenWibking BenWibking deleted the BenWibking/openpmd-output branch December 13, 2023 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I/O input/output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants