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

Add a DLD component #103

Merged
merged 4 commits into from
Apr 25, 2024
Merged

Add a DLD component #103

merged 4 commits into from
Apr 25, 2024

Conversation

philsmt
Copy link
Collaborator

@philsmt philsmt commented Jan 5, 2024

For various delay line detectors in use at SQS, the offline calibration pipeline is used to digitize the analog signals recorded with digitizers and reconstruct them into proper detector hits in time and space as a virtual detector source. As this kind of data is variable-length by nature, it is saved in sufficiently large fixed-length arrays filled with np.nan and compressed away.

While this data format is very efficient for storage and interoperable, it is somewhat difficult to load and work on in a pulse or hit-based manner. Based on observations of existing user code, this component transform the train/pulse/hit-shaped data into a linear pd.Series or pd.DataFrame with multi indices for convenient access.

It integrates into the existing pulse pattern components, though by default will use the internal pulse information saved as part of the processing result.

@philsmt
Copy link
Collaborator Author

philsmt commented Jan 10, 2024

Reminder to myself: Add pulse height information to edges, if available.

@philsmt
Copy link
Collaborator Author

philsmt commented Feb 20, 2024

@JamesWrigley Could I ask you as well to have a brief look? It's been dangling for a bit now, and I would like to base more components off it.

Copy link
Member

@JamesWrigley JamesWrigley left a comment

Choose a reason for hiding this comment

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

I should say that I'm completely unfamiliar with DLD's, so I may have missed something 👀

src/extra/components/dld.py Show resolved Hide resolved
src/extra/components/dld.py Outdated Show resolved Hide resolved
src/extra/components/dld.py Show resolved Hide resolved
docs/components/dld.md Show resolved Hide resolved
Copy link
Member

@JamesWrigley JamesWrigley left a comment

Choose a reason for hiding this comment

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

LGTM!

@takluyver
Copy link
Member

I found the _get_reduced_pd method not especially easy to read. It feels like it's trying to cover a lot of possibilities for something that's only called in 3 places. I suspect that it could be broken up a bit, but I haven't understood it well enough to suggest how. That's not really important if you're happy with it, though.

I think my question about RUN values is worth thinking about before we merge this.

@philsmt
Copy link
Collaborator Author

philsmt commented Feb 27, 2024

Do you feel strongly about the complexity of _get_reduced_pd? I also dislike the split_axis part, but ultimately it made the getter methods shorter. For now it's a private method anyway, though I could imagine it being more generally useful at some point.
I experimented with splitting it up in two methods, one for building an aligned index and one to reduce sparse datasets into dataframes, but it ended up clogging the getter methods...

@takluyver
Copy link
Member

No, I don't feel strongly about it. 🙂

@philsmt
Copy link
Collaborator Author

philsmt commented Apr 16, 2024

@takluyver I ended up giving it another try to reduce the complexity of _get_reduced_pd by splitting it along the messy "it-may-be-a-list-or-not" line. Is this more readable and/or understandable?

Copy link
Member

@takluyver takluyver left a comment

Choose a reason for hiding this comment

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

Thanks, that does look easier to read now.

src/extra/components/dld.py Outdated Show resolved Hide resolved
src/extra/components/dld.py Outdated Show resolved Hide resolved
src/extra/components/dld.py Outdated Show resolved Hide resolved
src/extra/components/dld.py Outdated Show resolved Hide resolved
@philsmt
Copy link
Collaborator Author

philsmt commented Apr 25, 2024

Thanks for review!

@philsmt philsmt merged commit 0570d89 into master Apr 25, 2024
6 checks passed
@philsmt philsmt deleted the feat/dld-detector branch April 25, 2024 12:06
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.

None yet

3 participants