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

Implementation of position attribute of Fields #682

Closed
skuschel opened this issue Dec 21, 2023 · 5 comments
Closed

Implementation of position attribute of Fields #682

skuschel opened this issue Dec 21, 2023 · 5 comments
Labels

Comments

@skuschel
Copy link

Description

Same as #681 but for the field attribute position.

I checked the field attributes Ex, Ey, Bx, By, Bx_m, By_m, Jx, Jy and the attribute position was always (0,0) for the same 2d test case as in #681 . position is supposed to contain information of the spatial stagger.

See also https://github.com/openPMD/openPMD-standard/blob/latest/STANDARD.md#mesh-based-records

To reproduce run the sample simulation with smilei v5.0 and check the output of $ h5ls -rvd Fields0.h5/data/0000001800/ | grep -4 position

So do the position attributes contain wrong values or are ALL outputs really defined on the front cell boundary before being dumped? I filed this as a bug, because it was a bug for the temporal stagger, but maybe its correct here. I did not find where the spatial stagger of outputs is defined.

@skuschel skuschel added the bug label Dec 21, 2023
@mccoys
Copy link
Contributor

mccoys commented Dec 21, 2023

Thank you for both reports. The attributes are indeed incorrect and will be fixed.

@mccoys
Copy link
Contributor

mccoys commented Jan 18, 2024

I read the standard again, and I find it very confusing. It does not specify that this is about staggering. There is a confusing way to refer to cells or elements. I would not have guessed this was about grid staggering. Can you confirm that the staggered direction must have a value of 0.5 in this attribute?

@mccoys
Copy link
Contributor

mccoys commented Jan 18, 2024

I have made changes locally to fix both the position issue and the timeOffset issue. Does this look ok to you?

$ h5ls -rvd Fields0.h5/data/0000001800/Bx_m | grep -4 timeOffset
    Attribute: position {2}
        Type:      native double
        Data:
               0, 0.5
    Attribute: timeOffset scalar
        Type:      native double
        Data:
               0.0785398163397448
    Attribute: unitDimension {7}
$ h5ls -rvd Fields0.h5/data/0000001800/Ex | grep -4 timeOffset
    Attribute: position {2}
        Type:      native double
        Data:
               0.5, 0
    Attribute: timeOffset scalar
        Type:      native double
        Data:
               0
    Attribute: unitDimension {7}

@skuschel
Copy link
Author

Thanks for the quick fix, looks good to me! Regarding your queston: Yes, the position attribute is related either to the stagger or to the (half) iteration when the fields are dumped. Some codes perform half iterations to make sure that the fields are dumped without offset, some codes dont and this is what is saved here. Its designed in its own variable to make sure this possibly tiny offset is always resolved numerically, even if the absolute coordinates are large values. The definition is here:

The following attributes must be stored with each scalar record and each component of a vector record:

position
    type: 1-dimensional array of N (floatX) where N is the number of dimensions in the simulation.
    range of each value: [ 0.0 : 1.0 )
    description: relative position of the component on the current element of the mesh/grid/node/cell/voxel; 0.0 means at the beginning of the mesh element and 1.0 is the beginning of the next mesh element; the same dimensionality N as in gridSpacing and gridGlobalOffset

from https://github.com/openPMD/openPMD-standard/blob/latest/STANDARD.md#mesh-based-records

In Contrast the gridGlobalOffset, which also has to be stored on every mesh record, can set arbitrary offsets for the entire grid, for example a moving window can be saved by only changing this number. Does that make sense?

mccoys pushed a commit that referenced this issue Jan 22, 2024
@mccoys
Copy link
Contributor

mccoys commented Jan 26, 2024

Fixed: 12dd7db

@mccoys mccoys closed this as completed Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants