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

FIO file read support #3539

Merged
merged 13 commits into from
Nov 2, 2021
Merged

FIO file read support #3539

merged 13 commits into from
Nov 2, 2021

Conversation

tifuchs
Copy link
Contributor

@tifuchs tifuchs commented Oct 15, 2021

Hello everybody,

as discussed in issue #3438 I have further improved my code, which allows silx.io to read FIO files generated by DESY beamlines.

This PR includes:

  • The FIO file reader (fioh5.py), implementing the io.commonh5 API so that the fio file is exposed as a Nexus file.
  • test code for the file reader (test_fioh5.py). At the moment, this only inculdes the basic read functionality.
  • update to the sphinx doc for a very basic API documentation

There are also small changes to the common silx.io packages, which allow the fio files to be opened with silx.io.open and silx convert.

While writing the code, I have encountered some issues with string datatypes.
In particular, there seems to be no matching datatype in silx for the h5py type "variable length strings" in 1d h5-like datasets. This is not really surprising since all data (in for example spech5) is internally stored as numpy arrays and those only support fixed length strings. My current solution is to read these data as python objects, which seems to work fine. But this might cause issues in the future. Is there a better solution?

Edit (after properly reading the h5py documentation ;) ):
The usage of the 'O' dtype is the recommended way of reading variable length strings for h5py as of version 3.0. So everything is fine.
I have also added support for the old API (< 3.0) in 421f70f.

I hope that you are happy with my first PR.

Best regards,
Timo

@t20100 t20100 added this to the 1.0.0 milestone Oct 19, 2021
@t20100
Copy link
Member

t20100 commented Oct 19, 2021

Great! Thanks for the PR!
I can't review it now, but I flagged it for the upcoming release 1.0.

Copy link
Member

@t20100 t20100 left a comment

Choose a reason for hiding this comment

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

Thanks againfor the PR!

I made a few minor comments.

src/silx/io/utils.py Outdated Show resolved Hide resolved
src/silx/io/fioh5.py Outdated Show resolved Hide resolved
src/silx/io/fioh5.py Show resolved Hide resolved
src/silx/io/fioh5.py Outdated Show resolved Hide resolved
src/silx/io/fioh5.py Outdated Show resolved Hide resolved
@tifuchs tifuchs requested a review from t20100 October 28, 2021 20:35
Copy link
Member

@t20100 t20100 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again for the PR!

@t20100 t20100 merged commit 84e2716 into silx-kit:master Nov 2, 2021
@tifuchs
Copy link
Contributor Author

tifuchs commented Nov 4, 2021

Great, I guess this
closes #3438

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants