Make "time", "dt", "timeOffset" and "timeUnitSI" optional #201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ax3l
reviewed
Nov 1, 2018
| @@ -233,9 +233,10 @@ Required Attributes for the `basePath` | |||
| -------------------------------------- | |||
There was a problem hiding this comment.
The title in line 232 still reads "Required" :)
Do we want to make this optional or recommended?
| @@ -661,6 +662,7 @@ Reminder: for scalar records the `record` itself is also the `component`. | |||
|
|
|||
| - `timeOffset` | |||
| - type: *(floatX)* | |||
| - scope: *optional* (only used if `time` and `dt` are used) | |||
There was a problem hiding this comment.
Maybe let's move this from Required for each Record to a section optional (or recommended if we decide so).
ax3l
changed the title
ax3l
reviewed
Nov 1, 2018
| files (`fileBased`) or series of groups (`groupBased`) can have | ||
| attributes that describe the current time and the last time step. (In the | ||
| case where each iteration corresponds to a different time.) Thus, the | ||
| following attributes are *optional*: | ||
|
|
||
| - `time` | ||
| - type: *(floatX)* |
There was a problem hiding this comment.
let's fix the typo in line 264 at the same time: conversion factor
There was a problem hiding this comment.
timeUnitSI should be made optional as well but required if time or dt is set
ax3l
changed the title
ax3l
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes the attributes "time", "dt", "timeOffset" and "timeUnitSI" optional.
Implements issue: #161
Description
As detailed description as possible.
What is introduced, removed or renamed and why?
What is made required, recommended, optional?
What concept stands behind this change?
Please also add an example.
Affected Components
baseLogic Changes
Which logic changes are conceptually introduced?
@RemiLehe please describe how a reader should handle time now. What if only few of the attributes are present? What if a mesh is read that has a temporal axis? #193 Or a particle record with
unitDimensiontime? What are defaults in such a case and what quantities are additive?Writer Changes
The above attributes are now optional
openPMD-api: https://github.com/openPMD/openPMD-api/...Reader Changes
Readers cannot explicitly assume that these attributes are present.
openPMD-validator: https://github.com/openPMD/openPMD-validator/...openPMD-viewer: https://github.com/openPMD/openPMD-viewer/...yt: https://github.com/yt-project/yt/...VisIt: https://github.com/openPMD/openPMD-visit-pluginopenPMD-api(upcoming): https://github.com/openPMD/openPMD-api/...XDMF(upcoming): https://github.com/openPMD/openPMD-converter/...Data Updater
Not needed.