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

Update Doros for new entries #19

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Update Doros for new entries #19

merged 1 commit into from
Oct 2, 2024

Conversation

JoschD
Copy link
Member

@JoschD JoschD commented Sep 30, 2024

The Doros team added more entries on the root-level of the HDF5 files.
Before, there were only BPMs and METADATA and the BPMs were identified by not being METADATA.
As now there is also TIMESTAMPS_INDEX and TIMESTAMPS_TABLE (I told Manuel this maybe should go into METADATA, as METADATA itself seems to be empty anyway... but so far they have not moved it).

I considered going by name (e.g. find _DOROS in the name) but then we are limited to which machine data we can write into this format.
I now identify BPMs as entries that have a subgroup nbOrbitSamplesRead which should be pretty robust.

Added a test with a reduced version of the new data (script to generate the data is also attached).

@JoschD JoschD added Type: Bug Something isn't working as it should. Type: Release Issue preparing for a release. Status: Review Needed Work currently stopped, untils someone else reviews it. Priority: High Work on this! Estimate: Normal Straightforward, but might require some time. Probably needs additional tests. labels Sep 30, 2024
@JoschD JoschD self-assigned this Sep 30, 2024
@JoschD
Copy link
Member Author

JoschD commented Sep 30, 2024

PS: with regards to the h5repack: This seems to be the only way to actually delete data from a hdf5 file (https://stackoverflow.com/questions/1124994/removing-data-from-a-hdf5-file). One should be aware of this. Quite annoying...

Copy link
Member

@fsoubelet fsoubelet left a comment

Choose a reason for hiding this comment

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

Regarding the h5repack thing: how about we just create a new HDF5 file from scratch, and write into it the relevant parts? This way without copying first we don't have to worry about data deletion in HDF5. What do you think?

turn_by_turn/doros.py Show resolved Hide resolved
tests/inputs/doros_shrink_data.py Show resolved Hide resolved
tests/inputs/doros_shrink_data.py Show resolved Hide resolved
tests/inputs/doros_shrink_data.py Show resolved Hide resolved
@JoschD
Copy link
Member Author

JoschD commented Oct 2, 2024

Regarding the h5repack thing: how about we just create a new HDF5 file from scratch, and write into it the relevant parts? This way without copying first we don't have to worry about data deletion in HDF5. What do you think?

I was thinking about it, and even trying it ... but then you kind of need to know what the other data is, that is in the file and how many "levels" deep it is in the hdf5, as you cannot simply copy a whole "tree" into another file. And if we build up a fake-file from scratch, we might miss something that they (the DOROS people) put in that make the reader fail (as was the case with the new entries leading to this PR).
This script is only used in case we ever need to update the test-file anyway. I don't care if it crashes or is not the most optimal. I wanted a quick way to reduce the 20-30MB DOROS files while keeping their internal structure as intact as possible.

Copy link
Member

@fsoubelet fsoubelet left a comment

Choose a reason for hiding this comment

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

In this case I'm all ok with this PR, thanks!

@JoschD JoschD merged commit 4fb38eb into master Oct 2, 2024
18 checks passed
@JoschD JoschD deleted the doros_update branch October 2, 2024 09:36
@JoschD JoschD removed the Status: Review Needed Work currently stopped, untils someone else reviews it. label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Estimate: Normal Straightforward, but might require some time. Probably needs additional tests. Priority: High Work on this! Type: Bug Something isn't working as it should. Type: Release Issue preparing for a release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants