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

added openPMD-api support for I/O #3666

Open
wants to merge 27 commits into
base: development
Choose a base branch
from

Conversation

guj
Copy link

@guj guj commented Dec 13, 2023

Summary

The goal of this pull request is to enable I/O support through the openPMD-api library

The proposed changes:

  • add new capabilities to AMReX

Src/Base/AMReX_PlotFileUtil.H Outdated Show resolved Hide resolved
}
}

AMReX_PtlCounter m_PtlCounter;
Copy link
Member

Choose a reason for hiding this comment

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

This would result in multiple definitions if this header is included in more than one translation units.

Copy link
Author

Choose a reason for hiding this comment

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

This eventually is a member variable of class ParticleContainer_impl, as this file is included in "AMReX_ParticleContainer.H".

Copy link
Member

Choose a reason for hiding this comment

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

Instead of plain text including this into another class, which will complicate our transition to C++ modules and is a bit surprising in general, consider making this a C++ mixin class:
https://en.wikipedia.org/wiki/Mixin

Simply derive ParticleContainer_impl from this.

Copy link
Author

Choose a reason for hiding this comment

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

The extra code in this file is a function that figures out ptl offsets for each processors. I think using mixin possibly will be an overkill, and clients need to change their code when initialing ParticleContainer_impl related classes.

Since this is only used by openPMD-api, I will move this function into the amex::openpmd_api scope. Leave the AMReX_ParticleContainer.H as is.

Copy link
Member

@WeiqunZhang WeiqunZhang left a comment

Choose a reason for hiding this comment

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

I actually have more changes to fix various compiler warnings. Do you want me to post as comments or push to your branch?

Src/Extern/openPMD-api/AMReX_ParticlesOPENPMD.H Outdated Show resolved Hide resolved
Src/Extern/openPMD-api/AMReX_ParticlesOPENPMD.H Outdated Show resolved Hide resolved
Src/Extern/openPMD-api/AMReX_ParticlesOPENPMD.H Outdated Show resolved Hide resolved
Src/Extern/openPMD-api/AMReX_PlotFileUtilOPENPMD.H Outdated Show resolved Hide resolved
@guj
Copy link
Author

guj commented Jan 5, 2024

I actually have more changes to fix various compiler warnings. Do you want me to post as comments or push to your branch?

Just saw you pushed. Thanks.

set default axis labels to be "x/y/z" for 1/2/3-Dims
other behaviours should be done in application codes
removed print statements
.github/workflows/clang.yml Outdated Show resolved Hide resolved
Src/Extern/openPMD-api/AMReX_ParticlesOPENPMD.H Outdated Show resolved Hide resolved
Src/Extern/openPMD-api/AMReX_PlotFileUtilOPENPMD_PTLImpl.H Outdated Show resolved Hide resolved
Tools/GNUMake/packages/Make.openpmd Outdated Show resolved Hide resolved
Tools/CMake/AMReX_Config_ND.H.in Outdated Show resolved Hide resolved
Src/CMakeLists.txt Outdated Show resolved Hide resolved
.github/workflows/clang.yml Outdated Show resolved Hide resolved
Comment on lines +31 to +32
m_PtlCounter.m_MPISize = amrex::ParallelDescriptor::NProcs();
m_PtlCounter.m_MPIRank = amrex::ParallelDescriptor::MyProc();
Copy link
Member

Choose a reason for hiding this comment

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

AMReX includes for ParallelDescriptor, ParallelGather, <vector> missing in this file.

Copy link
Author

Choose a reason for hiding this comment

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

This file is currently embedded in AMReX_ParticleContainer.H, which has all the included files declared already.

}
}

AMReX_PtlCounter m_PtlCounter;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of plain text including this into another class, which will complicate our transition to C++ modules and is a bit surprising in general, consider making this a C++ mixin class:
https://en.wikipedia.org/wiki/Mixin

Simply derive ParticleContainer_impl from this.

Comment on lines 1358 to 1360
#ifdef AMREX_USE_OPENPMD_API
#include "AMReX_ParticlesOPENPMD.H"
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if this was used as a mixin class instead of a textual include.

guj and others added 8 commits February 22, 2024 09:22
Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
remove commented code

Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
reduce AMREX_USE_OPENPMD_API to  AMREX_USE_OPENPMD

Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
reduce AMReX_OPENPMD_API to AMReX_OPENPMD

Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
AMREX_USE_OPENPMD instead of AMREX_USE_OPENPMD_API

Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
Use AMReX_OPENPMD instead of AMReX_OPENPMD_API

Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
Copy link
Member

@WeiqunZhang WeiqunZhang left a comment

Choose a reason for hiding this comment

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

I don't think this PR could break any existing codes. Since this is big PR and adds a new feature, I am okay with merging it 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.

3 participants