Skip to content

Always use all selected trains in pulse pattern components#434

Open
philsmt wants to merge 3 commits intomasterfrom
fix/pulsepattern-train-ids
Open

Always use all selected trains in pulse pattern components#434
philsmt wants to merge 3 commits intomasterfrom
fix/pulsepattern-train-ids

Conversation

@philsmt
Copy link
Copy Markdown
Collaborator

@philsmt philsmt commented Dec 19, 2025

PulsePattern-based components track train IDs in addition to pulse IDs to handle trains that don't have any pulses, e.g. in methods like pulse_counts(). Unfortunately I noticed only now that all implementations actually only account for trains that have data (in the sense of pulse records, not whether there are actually any pulses). This typically happened by using KeyData.train_id_coordinates() rather than KeyData.train_ids.

This PR fixes this by always using all selected train IDs whether data records exist or not. While I would claim this is the correct behaviour as done in EXtra-data (e.g. KeyData.data_counts()), it is somewhat changing the behaviour. I also had to fix a test to account for this, though it clearly did the wrong thing.

As part of this, I realized all existing implementations of _get_train_ids() can actually be handled by the default implementation in PulsePattern.

@philsmt philsmt requested a review from JamesWrigley December 19, 2025 13:55
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.78%. Comparing base (f57e71c) to head (728c621).

Files with missing lines Patch % Lines
src/extra/components/pulses.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
- Coverage   72.83%   72.78%   -0.05%     
==========================================
  Files          35       35              
  Lines        6048     6048              
==========================================
- Hits         4405     4402       -3     
- Misses       1643     1646       +3     

☔ 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

@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! I have a soft preference for holding off until the next environment (https://github.com/European-XFEL/EXtra/milestone/2) but I'll leave the decision to you.

@philsmt philsmt added the breaking change Something to merge in between cycles when the last cycle's release was tagged label Dec 23, 2025
@philsmt
Copy link
Copy Markdown
Collaborator Author

philsmt commented Dec 23, 2025

Yes, I think that's reasonable. I created a breaking change tag for this (and another PR already waiting for it), so we can locate them quickly.

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.

2 participants