Skip to content

Fix reading of vector attributes with only one contained value#1085

Merged
ax3l merged 9 commits intoopenPMD:devfrom
franzpoeschel:fix-single-value-vector-attributes
Aug 11, 2021
Merged

Fix reading of vector attributes with only one contained value#1085
ax3l merged 9 commits intoopenPMD:devfrom
franzpoeschel:fix-single-value-vector-attributes

Conversation

@franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Aug 10, 2021

Noticed by @PrometheusPi
Some of our backends can't distinguish attributes of scalar types and attributes of vector types containing only a single value
We already check for that situation in some places, in others we forget it.

We will keep forgetting it, so this PR modifies the Attribute::get() function template to do some more automatic conversions:

  • Convert from scalar values to vectors with a single value
  • Convert between std::vector and std::array types (some backends cannot distinguish those either, so I'll take the chance to cover that)

First commit has a failing test, I'll push the fix afterwards. I will also take the freedom to do some simplifications in Attribute.hpp.

TODO:

  • Remove those manual checks that are scattered across our code base?

1) single values to 1-value vectors
2) vectors to arrays
3) arrays to vectors
1) Simplify types in DoConvert, remove unnecessary template parameter
2) Replace a long if-then-else chain by variantSrc::visit
Make more widely compile-able.
@ax3l
Copy link
Member

ax3l commented Aug 11, 2021

Thanks, good idea!

Remove those manual checks that are scattered across our code base?

👍

CI: note that we see a double free in one of the tests currently.

@franzpoeschel franzpoeschel force-pushed the fix-single-value-vector-attributes branch from 7b7dde1 to 9f2bc40 Compare August 11, 2021 09:14
@franzpoeschel
Copy link
Contributor Author

CI: note that we see a double free in one of the tests currently.

It looks like icpc doesn't really understand variantSrc::visit. I've gone back to the old way for Intel compilers again.

@franzpoeschel
Copy link
Contributor Author

Remove those manual checks that are scattered across our code base?

Done that now, but I'm leaving this one in to stay on the safe side and always read unitDimension to its dedicated type.

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.

I think that's a good approach and the PR looks great, thanks a lot!
I'll just slightly modify the ICC guard and then it can go in.

As defined in CMake for compiler identification.
@ax3l ax3l enabled auto-merge (squash) August 11, 2021 20:06
@ax3l ax3l added this to the 0.14.2 milestone Aug 11, 2021
@ax3l ax3l merged commit 444db8f into openPMD:dev Aug 11, 2021
@ax3l
Copy link
Member

ax3l commented Aug 13, 2021

@franzpoeschel given the original issue that @PrometheusPi saw: what backend and attribute exactly was that?
Was that with PIConGPU?

@PrometheusPi
Copy link
Member

@ax3l This is triggered by 0.6.0-dev PIConGPU data created with openPMD-api using the adios2 backend. The error appeared when using the latest openPMD-viewer (see also discussion on SLACK):

In [3]: ts=OpenPMDTimeSeries('/__workdir__/jet_preexpansion_502/simOutput/openPMD/',backend='openpmd-api')
RuntimeError                              Traceback (most recent call last)

<ipython-input-3-b2fd3d9a7bec> in <module>
----> 1 ts=OpenPMDTimeSeries('/__workdir__/jet_preexpansion_502/simOutput/openPMD/',backend='openpmd-api')

~/.conda/envs/analysis/lib/python3.9/site-packages/openpmd_viewer/openpmd_timeseries/main.py in init(self, path_to_dir, check_all_files, backend)
     85 
     86         # - Extract parameters from the first file
---> 87         t, params0 = self.data_reader.read_openPMD_params(self.iterations[0])
     88         self.t[0] = t
     89         self.extensions = params0['extensions']

~/.conda/envs/analysis/lib/python3.9/site-packages/openpmd_viewer/openpmd_timeseries/data_reader/data_reader.py in read_openPMD_params(self, iteration, extract_parameters)
    135 
    136         elif self.backend == 'openpmd-api':
--> 137             return io_reader.read_openPMD_params(
    138                     self.series, iteration, extract_parameters)
    139 

~/.conda/envs/analysis/lib/python3.9/site-packages/openpmd_viewer/openpmd_timeseries/data_reader/io_reader/params_reader.py in read_openPMD_params(series, iteration, extract_parameters)
     75             metadata = {}
     76             metadata['geometry'] = field.get_attribute('geometry')
---> 77             metadata['axis_labels'] = field.axis_labels
     78 
     79             # Swap the order of the labels if the code that wrote the HDF5 file

RuntimeError: getCast: no cast possible.

Most likely caused by reading /data/0/fields/picongpu_idProvider/axisLabels but @franzpoeschel knows this better.

@ax3l
Copy link
Member

ax3l commented Aug 17, 2021

Clarified offline:

  • seen with idProvider (1d mesh)
  • axisLabels is only entry then
  • ADIOS1/2 attributes are always 1D arrays (of size 1-N)

ax3l pushed a commit to ax3l/openPMD-api that referenced this pull request Aug 18, 2021
…MD#1085)

* Failing test

* Conversions in Attribute.hpp

1) single values to 1-value vectors
2) vectors to arrays
3) arrays to vectors

* Some cleanup in Attribute.hpp

1) Simplify types in DoConvert, remove unnecessary template parameter
2) Replace a long if-then-else chain by variantSrc::visit

* CoreTest: Fix std::array constructors

Make more widely compile-able.

* Explicit casting in some places

This avoids some warnings

* Intel compilers: Don't use variantSrc::visit

They don't like it

* Remove scattered checks for vector attributes

* Generalize icpc guard

As defined in CMake for compiler identification.

* Doc ICC version (2021.3.0)
ax3l added a commit that referenced this pull request Aug 18, 2021
* Bug fix: Don't forget closing files (#1083)

* Failing test
* Bug fix: Don't forget closing files

* HDF5: Fix String Vlen Attribute Reads (#1084)

We inofficially try to also support HDF5 variable lengths strings in
reading, just to increase compatibility.

This was never working it seems.

* Fix reading of vector attributes with only one contained value (#1085)

* Failing test

* Conversions in Attribute.hpp

1) single values to 1-value vectors
2) vectors to arrays
3) arrays to vectors

* Some cleanup in Attribute.hpp

1) Simplify types in DoConvert, remove unnecessary template parameter
2) Replace a long if-then-else chain by variantSrc::visit

* CoreTest: Fix std::array constructors

Make more widely compile-able.

* Explicit casting in some places

This avoids some warnings

* Intel compilers: Don't use variantSrc::visit

They don't like it

* Remove scattered checks for vector attributes

* Generalize icpc guard

As defined in CMake for compiler identification.

* Doc ICC version (2021.3.0)

* setAttribute: Reject Empty Strings (#1087)

* setAttribute: Reject Empty Strings

Some backends, especially HDF5, do not allow us to define zero-sized
strings. We thus need to catch this in the frontend and forward the
restriction to the user.

* Test: setAttribute("key", "") throws

* setAttribute Check: C++14 compatible

* Don't read iterations if they have already been parsed (#1089)

* Release: 0.14.2

Co-authored-by: Franz Pöschel <franz.poeschel@gmail.com>
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