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

Integrate openpmd-scipp into the main API #1716

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Feb 4, 2025

This merges the code from https://github.com/pordyna/openpmd_scipp into the main API such that it can be used by series.to_scipp().

TODO:

  • Documentation (fields)
  • Documentation (particles) COMMENT: The TODO there refers to particles not being even implemented yet, I think?
  • Clean up the example
  • Have a look at the rest of what is in the repository, see what needs to / can be transferred
  • CI integration
  • Licensing headers

@franzpoeschel franzpoeschel requested a review from ax3l February 4, 2025 13:13
@franzpoeschel franzpoeschel self-assigned this Feb 4, 2025
@pordyna
Copy link
Contributor

pordyna commented Feb 4, 2025

I just took a quick look. This looks quite nice and wouldn't change much of the interface. The other stuff in the repo is mostly just for creating the README and testing, formatting etc. so I guess the only thing you would need to take over are the requirements, but this is only numpy and scipp. Also, it would make sense to make the example an integration test. Running the Jupyter notebook without verifying any outputs was also the only test I had.

@franzpoeschel
Copy link
Contributor Author

so I guess the only thing you would need to take over are the requirements, but this is only numpy and scipp

Also the Readme; I will try to integrate that into the Readthedocs page.
For the requirements, note that the code in this PR loads scipp lazily, so the main API can still be used without having that installed. numpy is already a hard requirement, but scipp should be at most an optional requirement (if there is a way to tell that to the Python packaging).

Also, it would make sense to make the example an integration test. Running the Jupyter notebook without verifying any outputs was also the only test I had.

I'd suggest that we add a test separately as part of APITest.py. The "target audience" for the examples are users who can use them as guidelines for their own code, so I would not clutter the example with testing code.

@franzpoeschel franzpoeschel force-pushed the add-scipp branch 3 times, most recently from 2986b4f to 294f890 Compare February 12, 2025 13:18
@@ -0,0 +1,340 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Is the notebook important to add or does the .py script + README have the same details?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ax3l the README from my repo is identical to the notebook since I was generating my README.md from it with:

rm -rf README_files
python -m nbconvert --to notebook --execute README.ipynb --output README.executed.ipynb
python -m nbconvert --to markdown README.executed.ipynb --output README.md - TagRemovePreprocessor.enabled=True --TagRemovePreprocessor.remove_cell_tags remove_cell
rm -f README.executed.ipynb

so you don't need both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added both since having this documentation page alternatively as an executable and interactive Notebook would be a useful extra for learning how to use this. It's of course not ideal for maintenance..


Ex_line.plot()

.. figure:: README_17_0.svg
Copy link
Member

Choose a reason for hiding this comment

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

Can you potentially upload the four .svg images to a public GH gist and use them by URL? That way, we do not add larger binary artifacts to our git repo here, which keeps it small on git clone for CMake superbuilds.

Copy link
Contributor Author

@franzpoeschel franzpoeschel Feb 28, 2025

Choose a reason for hiding this comment

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

SVG files are not binary, these are text files with each ~800 lines, less than many of our source files.
I tried putting them on a Gist, but it seems that Github does not hand out usable raw links.

Comment on lines 1 to 11
"""openpmd_scipp: A Python package for loading openPMD datasets
into scipp DataArrays.

See README.md for documentation

Author:
Pawel Ordyna <[email protected]>

License:
GPL - 3.0 license. See LICENSE file for details.
"""
Copy link
Member

Choose a reason for hiding this comment

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

@pordyna for openPMD-api, we use the LGPL license. Please confirm we can change the license for files in this PR and then please update the headers accordingly @franzpoeschel .

Copy link
Contributor

Choose a reason for hiding this comment

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

yes sure go for it!

License: LGPLv3+
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