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

Optionally use negative pulse indices for PPL-only DLD pulses #167

Merged
merged 3 commits into from
May 5, 2024

Conversation

philsmt
Copy link
Collaborator

@philsmt philsmt commented May 1, 2024

The PumpProbePulses component and its spiritual predecessor DldPulses combine FEL-only, PPL-only and pumped pulses into a single consistent pattern, i.e. may have more pulses than a pure FEL pattern in the presence of PPL-only pulses.

While this is useful for analysis, it presents problems when correlating FEL-only data using enumerated indices to label pulses. The XGM component e.g. will only ever look at pulses with FEL (either FEL-only or pumped) and enumerate accordingly, being incompatible with the integrated pattern.

While this can be fixed by using global pulse IDs (i.e. referring to indices in the machine pattern table), the use of pulse indices is still often desired and/or easier to obtain. This PR adds an optional flag to DldPulses (only for now, can be ported to PumpProbePulses) to modify the enumeration behaviour:

  • FEL-only and pumped pulses are enumerated without taking the PPL into account
  • PPL-only pulses are enumerated with negative indices, i.e. starting with -1 and counting down

The resulting indices are again compatible with PPL-unaware pulse indices, while still containing the PPL-only pulses.

Examples:

  • Four FEL pulses with two preceding PPL-only pulses:
    0, 1, 2, 3, 4, 5 becomes -1, -2, 0, 1, 2, 3
  • Four FEL pulses with two PPL-only pulses at start and end each
    0, 1, 2, 3, 4, 5, 6, 7 becomes -1, -2, 0, 1, 2, 3, -3, -4

Also fixes a minor bug with the preceding #166 to ensure indexes are compatible, and simplifies passing custom arguments to construct the internal pulse information with DelayLineDetector.

@philsmt philsmt requested a review from takluyver May 1, 2024 12:48
@philsmt philsmt force-pushed the feat/ppl-negative-pulse-indices branch 2 times, most recently from b628f4b to 1f4525e Compare May 1, 2024 13:05
@takluyver
Copy link
Member

When I first glanced at the title, I was thinking of a scenario where all your pump-only pulses come before the first X-ray pulse. In that specific case, it would be natural to use negative indices like:

[-4, -3, -2, -1, 0, 1, 2, 3]

Obviously that doesn't work neatly in other scenarios, like the one in your tests. But if this scenario does occur, I wonder if it will be confusing to people that the order is:

[-1, -2, -3, -4, 0, 1, 2, 3]

I don't think there's anything we can do about that, but I thought I'd mention it in case it gives you a great idea. 😉

Comment on lines +1497 to +1501
for i, tid, row in zip(range(len(triggers)), train_ids, triggers):
if tid != prev_tid:
prev_tid = tid
only_ppl_index = -1
with_fel_index = 0
Copy link
Member

Choose a reason for hiding this comment

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

There's probably a way to do this neatly with itertools.groupby() to have an outer loop over train IDs. It's also not a big deal, though - I know you want to get this out quickly, and I think this code works fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried

for train_id, vals in groupby(enumerate(triggers), lambda x: train_ids[x[0]]):
    only_ppl_index = -1
    with_fel_index = 0

    for i, trigger in vals:
        if trigger['ppl'] and not trigger['fel']:
            pulse_indices[i] = only_ppl_index
            only_ppl_index -= 1
        else:
            pulse_indices[i] = with_fel_index
            with_fel_index += 1

but it made the overall call to pulse_ids() about 15% slower. While I suspect the grouping itself is faster, the nested loops seem to end up taking more time due to constructing the necessary generators.

@takluyver
Copy link
Member

Other than that, LGTM

@philsmt philsmt force-pushed the feat/ppl-negative-pulse-indices branch from 1f4525e to 143e541 Compare May 5, 2024 15:11
@philsmt philsmt merged commit 27ad61e into master May 5, 2024
6 checks passed
@philsmt philsmt deleted the feat/ppl-negative-pulse-indices branch May 5, 2024 15:50
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

2 participants