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

GridSpacing: Per-Component UnitSI & UnitDimension #193

Merged
merged 5 commits into from
Nov 13, 2018

Conversation

RemiLehe
Copy link
Member

@RemiLehe RemiLehe commented Apr 23, 2018

Implements issue: Close #122

Description

This allows meshes to have axes with different dimensions (e.g. for phase space data where one axis is a position, and the second axis is a momentum.)

What is introduced, removed or renamed and why?

The attributes gridUnitDimension and gridUnitSI are now N times longer, where N is the dimension of the mesh.
In particular gridUnitDimension is a 1D array of length 7*N. This was preferred over a 2D array of dimension (N,7) since it seems that h5py does not support multidimensional arrays as attributes.

Affected Components

  • base: All mesh data

Writer Changes

How does this change affect data writers?
Data writers should now write the longer array for gridUnitSI and add gridUnitDimension.

Note: We could make this backward compatible by saying that, if the writer wrote the previous short-array format, then this applies to each axis... @ax3l Is this worth it or too confusing. -> too confusing, we have the updater for that :)

Reader Changes

How does this change affect data readers?
Data readers should now extract the dimension of each axis.

Data Converter

How does this affect already existing files with previous versions of the standard?
For existing attributes should be duplicated N times, in order to convert to the new format.

@RemiLehe RemiLehe added the major change non-backwards compatible change label Apr 23, 2018
@ax3l ax3l self-requested a review April 24, 2018 13:02
@ax3l
Copy link
Member

ax3l commented Apr 24, 2018

Note: We could make this backward compatible by saying that, if the writer wrote the previous short-array format, then this applies to each axis...

Very good point. I think it's too special to allow this short-cut which just adds extra-checks in readers and writers. So let's not allow to just write it once as it was allowed before.

STANDARD.md Outdated
@@ -95,7 +95,7 @@ Each file's *root* group (path `/`) must at least contain the attributes:
present standard (e.g. fields on an unstructured mesh),
this can be done be storing this data within a path that *is not*
of the form given by `basePath` (e.g. `/extra_data`). In this
way, the openPMD parsing tools will not parse this additional data.
way, the openPMD parsing tools will not parse this additional data.
Copy link
Member

Choose a reason for hiding this comment

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

while we are on it: it's not only the openPMD parsing tools but general openPMD data readers

STANDARD.md Outdated
- examples:
- For a 2D spatial grid (`L=1`), store array
`(1., 0., 0., 0., 0., 0., 0., 1., 0., 0., 0., 0., 0., 0.)`
- For a 2D phase space (Unit "m" along the first axis
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we better stay with L and M * L / T as defined in unitDimension?

STANDARD.md Outdated
@@ -424,11 +424,29 @@ meshes):
- example: `(0.0, 100.0, 0.0)` or `(0.5, 0.5, 0.5)`

- `gridUnitSI`
- type: *(float64 / REAL8)*
- type: 1-dimensional array containing N *(float64 / REAL8)*
elements, where N is the number of dimensions in the simulation
Copy link
Member

@ax3l ax3l Apr 24, 2018

Choose a reason for hiding this comment

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

This is related to #125 & #129 (PR in #194): if we flatten the array we have to specify how.
I would propose to use the wording from #129 as such:
, dimensions shall be ordered from fastest to slowest varying index [of the described mesh].

Copy link
Member

Choose a reason for hiding this comment

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

update: we probably will go from slowest to fastest in #194

STANDARD.md Outdated

- `gridUnitDimension`
- type: array of 7 N *(float64 / REAL8)*
elements, where N is the number of dimensions in the simulation
Copy link
Member

Choose a reason for hiding this comment

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

we need to add the same comment with order as above for flattening

STANDARD.md Outdated
- example: `(1.0e-9, 1.0e-9, 1.0e-6)`

- `gridUnitDimension`
- type: array of 7 N *(float64 / REAL8)*

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as resolved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My thought would be to not even mandate any precision (ie just say "real"). For example, why should the standard disallow the use of real4?

Copy link
Member

@ax3l ax3l Nov 7, 2018

Choose a reason for hiding this comment

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

The unit dimension attribute is not a record but just describing the dimensionality ("unit"). It introduces no problem keeping it fixed and makes parsing very simple.

Copy link
Member

@ax3l ax3l Nov 7, 2018

Choose a reason for hiding this comment

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

@RemiLehe pls replace array with 1-dimensional array for consistency.

Suggested change
- type: array of 7 N *(float64 / REAL8)*
- type: 1-dimensional array of 7 N *(float64 / REAL8)*

Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Annotated the changed index order which is introduced in #194

STANDARD.md Outdated
@@ -425,29 +425,30 @@ meshes):

- `gridUnitSI`
- type: 1-dimensional array containing N *(float64 / REAL8)*
elements, where N is the number of dimensions in the simulation
elements, where N is the number of dimensions in the simulation. Dimensions shall be ordered from fastest to slowest varying index of the described mesh.
Copy link
Member

Choose a reason for hiding this comment

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

will be inverted

STANDARD.md Outdated

- `gridUnitDimension`
- type: array of 7 N *(float64 / REAL8)*
elements, where N is the number of dimensions in the simulation. Dimensions shall be ordered from fastest to slowest varying index of the described mesh.
Copy link
Member

Choose a reason for hiding this comment

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

slowest to fastest

STANDARD.md Outdated
@@ -424,11 +424,30 @@ meshes):
- example: `(0.0, 100.0, 0.0)` or `(0.5, 0.5, 0.5)`

- `gridUnitSI`
- type: *(float64 / REAL8)*
- type: 1-dimensional array containing N *(float64 / REAL8)*
elements, where N is the number of dimensions in the simulation. Dimensions shall be ordered from fastest to slowest varying index of the described mesh.
Copy link
Member

Choose a reason for hiding this comment

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

slowest to fastest

Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Changes look great!

Let us just add the PR to the converter before we merge.

@PrometheusPi
Copy link
Member

@ax3l Does this require changing how we write hdf5 data in the phase space and radiation plugin in PIConGPU?

@ax3l
Copy link
Member

ax3l commented May 24, 2018

@PrometheusPi there is a whole bunch of changes that are changing openPMD in a incompatible way in 2.0.0. This is one of them and will enable that we can describe phase space and radiation plugin in valid openPMD.

When we have all changes incorporated, a changelog will summarize all of them again. Also, we can then use openPMD-api instead of libSplash to write from all PIConGPU plugins.

We will then update all places of PIConGPU and then add openPMD to phase space and radiation plugin.

STANDARD.md Outdated

- `gridUnitDimension`
- type: array of 7 N *(float64 / REAL8)*
elements, where N is the number of dimensions in the simulation. Dimensions shall be ordered from slowest to fastest varying index of the described mesh.
Copy link
Member

Choose a reason for hiding this comment

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

Overlong line. Maybe put "Dimensions shall be..." on the next line (indented to same block).

Copy link
Member

@ax3l ax3l Nov 7, 2018

Choose a reason for hiding this comment

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

I would try to make this more clear how the arrays are stored by writing:

"The order of the N 7-value arrays must be identical to the axes in axisLabels."

This makes it consistent with the other attributes above that have the same order and uses the same wording, also consistently with #194.

Suggested change
elements, where N is the number of dimensions in the simulation. Dimensions shall be ordered from slowest to fastest varying index of the described mesh.
elements, where N is the number of dimensions in the simulation.
The order of the N 7-value arrays must be identical to the axes in `axisLabels`.

STANDARD.md Outdated
@@ -430,11 +430,30 @@ In addition, the following attribute is *recommended* (see the section
on Unit Systems and Dimensionality, further below):

- `gridUnitSI`
- type: *(float64 / REAL8)*
- type: 1-dimensional array containing N *(float64 / REAL8)*
elements, where N is the number of dimensions in the simulation. Dimensions shall be ordered from slowest to fastest varying index of the described mesh.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
elements, where N is the number of dimensions in the simulation. Dimensions shall be ordered from slowest to fastest varying index of the described mesh.
elements, where N is the number of dimensions in the simulation.
The order of the N values must be identical to the axes in `axisLabels`.

@ax3l ax3l changed the title Close #122 GridSpacing: Per-Component UnitSI & UnitDimension GridSpacing: Per-Component UnitSI & UnitDimension Nov 8, 2018
Apply changes as approved in last VC :)
@ax3l ax3l merged commit 8223062 into openPMD:upcoming-2.0.0 Nov 13, 2018
@@ -430,11 +430,32 @@ In addition, the following attribute is *recommended* (see the section
on Unit Systems and Dimensionality, further below):

- `gridUnitSI`
- type: *(float64 / REAL8)*
- type: 1-dimensional array containing N *(float64 / REAL8)*

Choose a reason for hiding this comment

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

Note: This does not extend, but instead changes the definition of gridUnitSI. According to this wording, writing a single scalar to be used for all axes will no longer be supported for 2.0, meaning that all writers need to change this.
Do we additionally want to allow scalars here in 2.0, or should writers migrate?

Currently implementing this by printing a warning for legacy-defined scalar values for gridUnitSI in openPMD/openPMD-api#1534:

Mesh &Mesh::setGridUnitSI(double gusi)
{
    setAttribute("gridUnitSI", gusi);
    if (auto standard = IOHandler()->m_standard;
        standard >= OpenpmdStandard::v_2_0_0)
    {
        std::cerr << "[Mesh::setGridUnitSI] Warning: Setting a scalar "
                     "`gridUnitSI` in a file with openPMD version '" +
                std::string(auxiliary::formatStandard(standard)) +
                "'. Consider specifying a vector instead in order to "
                "specify the gridUnitSI per axis (ref.: "
                "https://github.com/openPMD/openPMD-standard/pull/193).\n";
    }
    return *this;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major change non-backwards compatible change
Projects
Status: Implemented
Development

Successfully merging this pull request may close these issues.

6 participants