Skip to content

Remove per-row index level across components#362

Open
philsmt wants to merge 2 commits intomasterfrom
feat/remove-row-indices
Open

Remove per-row index level across components#362
philsmt wants to merge 2 commits intomasterfrom
feat/remove-row-indices

Conversation

@philsmt
Copy link
Copy Markdown
Collaborator

@philsmt philsmt commented Jul 11, 2025

This is an API breaking change to correct one of my really stupid ideas.

The first components using PulsePattern components to label data added another index level for every individual row, e.g. hitIndex for hits, signalIndex for signals etc. This was meant to have an easy way to look into the first hit of every pulse, e.g. At the same time, this vastly complicates indexing pulse-resolved data with each other as pandas trips over the additional index level. Without this level, it's straightforward to index pulse-resolved data both with a train-resolved index as well as pulse-resolved index. The original functionality of getting the first hit for each pulse can easily be achieved using .groupby() if needed.

I also stopped using this level in Timepix3, so this change now fixes this for DelayLineDetector and AdqRawChannel.

@philsmt philsmt force-pushed the feat/remove-row-indices branch 3 times, most recently from 3b5584d to 7b2edcf Compare July 11, 2025 09:36
@philsmt philsmt force-pushed the feat/remove-row-indices branch from 7b2edcf to 45c2c21 Compare July 11, 2025 12:02
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.20%. Comparing base (95c59de) to head (45c2c21).
⚠️ Report is 182 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
+ Coverage   57.87%   58.20%   +0.32%     
==========================================
  Files          31       31              
  Lines        4905     4962      +57     
==========================================
+ Hits         2839     2888      +49     
- Misses       2066     2074       +8     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
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.

I guess we can assume that not too many people have started using this code and relying on the extra index so far, so if there's any breakage beyond our own code, it should be minimal. 🤞

@philsmt
Copy link
Copy Markdown
Collaborator Author

philsmt commented Jul 14, 2025

I want to be honest here, this code has quite definitely been used over the past run in most applicable experiments. It is being advertised by example notebooks. I have not seen any use of this index level however, which coupled with the fact it makes indexing to trains/pulses harder is the reason I want to get rid of it. I waited for the end of the cycle/LIMP, so the 2025/1 environment remains compatible in case anyone relies on it.

@takluyver
Copy link
Copy Markdown
Member

OK, fair enough. I think we need to be careful with backwards compatibility, but it doesn't need to be 100% absolute, and it sounds like the benefits here outweigh the drawbacks. 👍

@philsmt
Copy link
Copy Markdown
Collaborator Author

philsmt commented Jul 14, 2025

Breaking compatibility is the intent here.

Maybe I should add a direct note to the method's documentation?

@takluyver
Copy link
Copy Markdown
Member

Sorry, I wasn't writing very clearly. I understand that this breaks compatibility, and I think it's an acceptable trade-off. 👍

@philsmt
Copy link
Copy Markdown
Collaborator Author

philsmt commented Jul 17, 2025

@takluyver Was that an LGTM? 🙃

@takluyver
Copy link
Copy Markdown
Member

LGTM

@philsmt
Copy link
Copy Markdown
Collaborator Author

philsmt commented Oct 20, 2025

I decided to defer merging this MR towards the end of the cycle, to prevent the small off chance to break someone's analysis in the middle of a cycle.

@JamesWrigley JamesWrigley added this to the 2026.1 milestone Oct 20, 2025
@philsmt philsmt added the breaking change Something to merge in between cycles when the last cycle's release was tagged label Dec 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Something to merge in between cycles when the last cycle's release was tagged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants