Skip to content

updated event_index and event_id description#1404

Merged
phyy-nx merged 4 commits intonexusformat:mainfrom
KyleQianliMa:update_NXdata_description
Apr 1, 2026
Merged

updated event_index and event_id description#1404
phyy-nx merged 4 commits intonexusformat:mainfrom
KyleQianliMa:update_NXdata_description

Conversation

@KyleQianliMa
Copy link
Copy Markdown
Contributor

Description

The description of event_index can be confusing for new developers. I added a few lines of description in event_index and event_id to make it more clear.

Details:

Event_index is associated with pulses. Each pulse creates an entry in event_index, the values of event_index points to event_id and event_id contains information about detector_id.

Example:
event_index:
0:0
1:1
2:1
3:1
4:3

Event_id:
0:490
1:231
2:434
3:611

This means at pulse 0, current detector bank detected an event 0, the det_id for this event is event_id[0] = 490. For pulse 1, a new event is detected and indexed 1, the det_id for this event is event_id[1] = 231. For pulse 2 and pulse 3 no events are detected on this detector bank therefore the event_index for these 2 pulses remain 1. For pulse 4, 2 events are detected, the last index of the detected event is 3, meaning event 2 and 3 are created from this pulse and their det_ids are registered as 434 and 611.

This PR added a short paragraph in event_index to illustrate the description above. Also a little bit in event_id to make it easier to understand.

@KyleQianliMa KyleQianliMa marked this pull request as draft September 10, 2024 17:20
@KyleQianliMa KyleQianliMa marked this pull request as ready for review September 10, 2024 17:20
@KedoKudo
Copy link
Copy Markdown

@phyy-nx Could you review this PR? Thanks

@KedoKudo
Copy link
Copy Markdown

@phyy-nx, is there any chance we can get this approved?

Copy link
Copy Markdown
Contributor

@phyy-nx phyy-nx left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, and sorry this was missed.

@phyy-nx
Copy link
Copy Markdown
Contributor

phyy-nx commented Mar 30, 2026

@PeterC-DLS do you know why the checks didn't run?

@PeterC-DLS
Copy link
Copy Markdown
Contributor

PeterC-DLS commented Mar 30, 2026

For safety's sake! I think you can try rebasing - see my reply to #1613 (comment)

@phyy-nx
Copy link
Copy Markdown
Contributor

phyy-nx commented Mar 30, 2026

Righto. @KyleQianliMa could you rebase onto master and force push your branch on your fork?

@KyleQianliMa
Copy link
Copy Markdown
Contributor Author

Righto. @KyleQianliMa could you rebase onto master and force push your branch on your fork?

I've merged with main. To run the workflow, do you need to approve it?

@phyy-nx
Copy link
Copy Markdown
Contributor

phyy-nx commented Mar 31, 2026

Whoops, the build error seems unrelated. Tagging @PeterC-DLS again :)

@PeterC-DLS
Copy link
Copy Markdown
Contributor

This sphinx-toolbox/sphinx-toolbox#201 is causing the CI issue. See my fix in #1628

@phyy-nx phyy-nx merged commit 85ad221 into nexusformat:main Apr 1, 2026
2 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in NIAC 2024 project Apr 1, 2026
@phyy-nx
Copy link
Copy Markdown
Contributor

phyy-nx commented Apr 1, 2026

Thanks all!

@KyleQianliMa KyleQianliMa deleted the update_NXdata_description branch April 1, 2026 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants