Refactor of LH5 I/O routines, deprecation of existing methods #24

MoritzNeuberger · 2023-10-30T09:50:23Z

#3
Second attempt to implement the refactoring of lh5 related classes and functions into a separate folder.
This refactoring also requires changes to the other relevant repositories (pygama, dspeed, legend-daq2lh5) as the include paths have changed. They will be included in pull requests for each package individually.

Some notes:

ls and show are still in the store.py file. They cannot be moved to utils.py as they reference the LH5Store object, which would result in a circular import. Should we try to move them to a separate file?
Is it okay to call the utility files both utils.py at different levels? I am not an expert enough to know if this could cause problems with the import.

codecov · 2023-10-30T09:51:46Z

Codecov Report

Attention: 152 lines in your changes are missing coverage. Please review.

Comparison is base (56298ed) 75.49% compared to head (ff8d627) 74.87%.

Files	Patch %	Lines
src/lgdo/lh5/store.py	84.50%	88 Missing ⚠️
src/lgdo/lh5/iterator.py	80.25%	31 Missing ⚠️
src/lgdo/lgdo_utils.py	0.00%	14 Missing ⚠️
src/lgdo/lh5_store.py	62.16%	14 Missing ⚠️
src/lgdo/lh5/utils.py	91.42%	3 Missing ⚠️
src/lgdo/utils.py	93.10%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
- Coverage   75.49%   74.87%   -0.62%     
==========================================
  Files          22       27       +5     
  Lines        2126     2209      +83     
==========================================
+ Hits         1605     1654      +49     
- Misses        521      555      +34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

MoritzNeuberger · 2023-10-30T11:38:13Z

Should I wait until this PR is reviewed before making the PR in the other repositories?

gipert · 2023-10-30T12:59:59Z

First of all: thanks a lot @MoritzNeuberger for taking care of this!

ls and show are still in the store.py file. They cannot be moved to utils.py as they reference the LH5Store object, which would result in a circular import. Should we try to move them to a separate file?

I would say so, yes.

Is it okay to call the utility files both utils.py at different levels? I am not an expert enough to know if this could cause problems with the import.

It's perfectly ok, in my opinion, since the scope is made clear by the subpackage name: lgdo.utils vs lgdo.lh5.utils.

Should we also:

rename LH5Iterator/LH5Store to just Iterator/Store?
Since this will break user code, keep it backward compatible for a while? Maybe just like we did in pygama?

Should I wait until this PR is reviewed before making the PR in the other repositories?

Yes, let's try to get this merged first.

gipert

For discussion: do we really want to drop the LH5 prefix in LH5Store and LH5Iterator? I like the lh5.Store and lh5.Iterator syntax more, but I wonder if the LH5 prefix could still be useful, if the names are individually imported.

Moritz: have you checked if there is any broken Sphinx reference left in files not affected by this PR? I guess it would be useful to turn on the Sphinx option to error/warn if cross-references are broken.

gipert · 2023-10-30T12:48:25Z

src/lgdo/lh5/iterator.py

+LGDO = Union[Array, Scalar, Struct, VectorOfVectors]
+
+
+class LH5Iterator(Iterator):


This should be renamed to Iterator in my opinion.

gipert · 2023-11-06T15:10:49Z

docs/source/notebooks/LH5Files.ipynb

@@ -101,7 +101,7 @@
   "id": "ef09f43c",
   "metadata": {},
   "source": [
-    "The group contains several LGDOs. Let's read them in memory. We start by initializing an `LH5Store` [[docs]](https://legend-pydataobj.readthedocs.io/en/stable/api/lgdo.html#lgdo.lh5_store.LH5Store) object:"
+    "The group contains several LGDOs. Let's read them in memory. We start by initializing an `Store` [[docs]](https://legend-pydataobj.readthedocs.io/en/stable/api/lgdo.html#lgdo.lh5_store.Store) object:"


Suggested change

"The group contains several LGDOs. Let's read them in memory. We start by initializing an `Store` [[docs]](https://legend-pydataobj.readthedocs.io/en/stable/api/lgdo.html#lgdo.lh5_store.Store) object:"

"The group contains several LGDOs. Let's read them in memory. We start by initializing an `Store` [[docs]](https://legend-pydataobj.readthedocs.io/en/stable/api/lgdo.html#lgdo.lh5.Store) object:"

All these links need to be fixed in the documentation...

gipert · 2023-11-06T15:18:32Z

src/lgdo/__init__.py

+    "Iterator",
+    "Store",


I guess we should remove these imports from the lh5 submodule, especially if we are dropping the LH5 prefix. The user should import lgdo.lh5.Store, and not lgdo.Store. But see my comment about this prefix in the review summary.

gipert · 2023-11-06T15:19:32Z

src/lgdo/cli.py

@@ -5,11 +5,11 @@

 import lgdo
 import lgdo.logging
-from lgdo import show
+from lgdo.lh5.store import show


Uhm I forgot to use relative imports in this file. Could you just import from . instead of lgdo?

gipert · 2023-11-06T15:36:25Z

src/lgdo/lh5/store.py

+            obj.resize(new_size=size)
+        return obj
+
+    def read_object(


I propose to rename read_object() and write_object() to just read() and write(). "object" sounds redundant to me.

MoritzNeuberger · 2023-11-17T17:59:32Z

I realized that I haven't made the change of read_object and write_object to read and write backwards compatible. Will do so asap.

jasondet · 2023-11-17T19:48:59Z

It looks like lgdo_utils.get_element_type and lgdo_utils.copy also both need bw compatibility calls?

Yeah it looks like you just need to go through each def and make sure there is a deprication warning and appropriate call-through. Thank you Moritz!

jasondet

just need to go through each def and make sure there is a deprication warning and appropriate call-through

gipert · 2023-11-21T15:30:08Z

lgdo_utils.copy will be deprecated once #30 is merged. I think it's also being barely used atm, @MoritzNeuberger you can just go ahead and remove it.

MoritzNeuberger · 2023-11-24T09:11:53Z

Hmm, seems like it didn't work with that approach. What I did was delete store.py while keeping the source in a separate file. Then I used git mv to move the lh5_store.py file into lh5 and committed. Then I changed the filename and committed again. Finally, I created a new lh5_store.py and filled both files with the appropriate content. When I run git log --follow store.py I get the full history, but I do not know how to keep the full history as it is not fully displayed on github.

docs/source/extensions/numbadoc.py

docs/source/notebooks/LH5Files.ipynb

src/lgdo/compression/radware.py

Co-authored-by: Luigi Pertoldi <gipert@pm.me>

gipert · 2023-11-24T13:28:19Z

This looks good to me, I will merge.

MoritzNeuberger changed the title ~~Issue 3 lh5 store refactor~~ lh5_store.py refactor (2nd attempt) Oct 30, 2023

MoritzNeuberger mentioned this pull request Oct 30, 2023

Support the latest legend-pydataobj and dspeed versions legend-exp/pygama#522

Merged

gipert requested review from gipert and jasondet October 30, 2023 13:00

gipert linked an issue Oct 30, 2023 that may be closed by this pull request

lh5_store.py refactor #3

Closed

gipert added this to the v2 milestone Oct 30, 2023

gipert added the lh5 HDF5 I/O label Nov 3, 2023

gipert requested changes Nov 6, 2023

View reviewed changes

jasondet approved these changes Nov 17, 2023

View reviewed changes

gipert changed the title ~~lh5_store.py refactor (2nd attempt)~~ Refactor of LH5 I/O routines, deprecation of existing methods Nov 23, 2023

MoritzNeuberger added 7 commits November 24, 2023 11:15

Moved lh5_store.py and renamed

94c944d

Overwrite content of store.py

0827ffb

Added other files

4c930a7

Added other files

76e892b

Moved test files

b55ae45

Changed test files

6c4acfc

Everything adjusted and passes tests

063fdb9

MoritzNeuberger force-pushed the issue_3_lh5_store_refactor branch from 227324d to 063fdb9 Compare November 24, 2023 10:41

MoritzNeuberger added 5 commits November 24, 2023 11:57

Also updated the docs agian

e1ac279

Renamed and back to and

c6a82c6

Renamed and back to and

9a05121

Adjusted warning messages to be individual for each function.

6df838c

Fixed docs

72cd1a3

gipert requested changes Nov 24, 2023

View reviewed changes

docs/source/extensions/numbadoc.py Outdated Show resolved Hide resolved

docs/source/notebooks/LH5Files.ipynb Outdated Show resolved Hide resolved

src/lgdo/compression/radware.py Outdated Show resolved Hide resolved

Apply suggestions from code review

ff8d627

Co-authored-by: Luigi Pertoldi <gipert@pm.me>

gipert approved these changes Nov 24, 2023

View reviewed changes

gipert merged commit 772fe17 into legend-exp:main Nov 24, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of LH5 I/O routines, deprecation of existing methods #24

Refactor of LH5 I/O routines, deprecation of existing methods #24

MoritzNeuberger commented Oct 30, 2023

codecov bot commented Oct 30, 2023 •

edited

Loading

MoritzNeuberger commented Oct 30, 2023

gipert commented Oct 30, 2023 •

edited

Loading

gipert left a comment

gipert Oct 30, 2023

gipert Nov 6, 2023

gipert Nov 6, 2023

gipert Nov 6, 2023

gipert Nov 6, 2023

MoritzNeuberger commented Nov 17, 2023

jasondet commented Nov 17, 2023

jasondet left a comment

gipert commented Nov 21, 2023

MoritzNeuberger commented Nov 24, 2023

gipert commented Nov 24, 2023

		LGDO = Union[Array, Scalar, Struct, VectorOfVectors]


		class LH5Iterator(Iterator):

	"The group contains several LGDOs. Let's read them in memory. We start by initializing an `Store` [[docs]](https://legend-pydataobj.readthedocs.io/en/stable/api/lgdo.html#lgdo.lh5_store.Store) object:"
	"The group contains several LGDOs. Let's read them in memory. We start by initializing an `Store` [[docs]](https://legend-pydataobj.readthedocs.io/en/stable/api/lgdo.html#lgdo.lh5.Store) object:"

Refactor of LH5 I/O routines, deprecation of existing methods #24

Refactor of LH5 I/O routines, deprecation of existing methods #24

Conversation

MoritzNeuberger commented Oct 30, 2023

codecov bot commented Oct 30, 2023 • edited Loading

Codecov Report

MoritzNeuberger commented Oct 30, 2023

gipert commented Oct 30, 2023 • edited Loading

gipert left a comment

Choose a reason for hiding this comment

gipert Oct 30, 2023

Choose a reason for hiding this comment

gipert Nov 6, 2023

Choose a reason for hiding this comment

gipert Nov 6, 2023

Choose a reason for hiding this comment

gipert Nov 6, 2023

Choose a reason for hiding this comment

gipert Nov 6, 2023

Choose a reason for hiding this comment

MoritzNeuberger commented Nov 17, 2023

jasondet commented Nov 17, 2023

jasondet left a comment

Choose a reason for hiding this comment

gipert commented Nov 21, 2023

MoritzNeuberger commented Nov 24, 2023

gipert commented Nov 24, 2023

codecov bot commented Oct 30, 2023 •

edited

Loading

gipert commented Oct 30, 2023 •

edited

Loading