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

Non-spatial meshes #1534

Merged
merged 22 commits into from
Nov 12, 2024
Merged

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Oct 5, 2023

Implements openPMD/openPMD-standard#193

TODO:

@franzpoeschel franzpoeschel added discussion api: new additions to the API labels Oct 5, 2023
@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Oct 18, 2023

This PR should be split into two parts at some point:

  1. Introduction of openPMD 2.0, together with some helper functions
  2. Introduction of non-spatial meshes

The first part might then also be relevant for other PRs where breaking changes are introduced (e.g. #1493 use of abbreviated attributes by default in JSON backend)

EDIT: Done #1551

@franzpoeschel franzpoeschel force-pushed the non-spatial-meshes branch 2 times, most recently from 7534cb8 to 6d7513b Compare December 22, 2023 18:29
@franzpoeschel franzpoeschel force-pushed the non-spatial-meshes branch 3 times, most recently from d184a23 to 6045e89 Compare February 29, 2024 12:41
src/Mesh.cpp Outdated Show resolved Hide resolved
@franzpoeschel franzpoeschel force-pushed the non-spatial-meshes branch 2 times, most recently from da28b8c to 12ed9bd Compare May 24, 2024 14:49
@franzpoeschel franzpoeschel force-pushed the non-spatial-meshes branch 2 times, most recently from 1e6bb68 to 227cb94 Compare October 29, 2024 12:05
@franzpoeschel franzpoeschel changed the title WIP: Non-spatial meshes Non-spatial meshes Oct 29, 2024
E = meshes["E"]
E.reset_dataset(io.Dataset(io.Datatype.DOUBLE, [10, 10, 10]))

def unsupported_in_1_1():

Check notice

Code scanning / CodeQL

Unused local variable Note test

Variable unsupported_in_1_1 is not used.
@ikbuibui
Copy link

ikbuibui commented Nov 7, 2024

I took a look over the PR and it seems okay to me. The interface fits for the binningPlugin use case. However the review isnt very thorough as I am unfamiliar with most of the code base and I don't know enough to follow some workflows.

@ikbuibui
Copy link

ikbuibui commented Nov 7, 2024

A small comment is that in the binningPlugin in PIConGPU i was using std::array<double, 7> to hold the dimensionality data. I see that the API requires vectors, even though it assumes the 7 SI Dimensions. Is it really required to be like this?

@franzpoeschel
Copy link
Contributor Author

Thank you for reviewing, Tapish!

A small comment is that in the binningPlugin in PIConGPU i was using std::array<double, 7> to hold the dimensionality data. I see that the API requires vectors, even though it assumes the 7 SI Dimensions. Is it really required to be like this?

Can you point me to where exactly you mean? Within the context of this PR, std::vector is used for n-dimensional data, i.e. for a 3-dimensional mesh, you would specify three numbers in Mesh &setGridUnitSI(std::vector<double> const &gridUnitSI).

Beyond that, we have always had two representations for Units: std::array<double, 7> and std::map<UnitDimension, double>. I have taken the chance of this PR to make this a bit more systematic and provide setters of both kinds, as well as conversion functionality.

With this PR, std::vector may now additionally used to specify the unit per dimension: in Mesh &setGridUnitDimension(unit_representations::AsArrays const &gridUnitDimension);, the type AsArrays resolves to std::vector<std::array<double, 7>>. So, the use of std::array mostly stays.

@ikbuibui
Copy link

Yes, you are right. It was my misunderstanding. Looks good!

@franzpoeschel franzpoeschel merged commit d1015ee into openPMD:dev Nov 12, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: new additions to the API discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants