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

Feature/ehinkle hip selection #97

Open
wants to merge 41 commits into
base: develop
Choose a base branch
from

Conversation

edhinkle
Copy link
Member

Adding tracklets (updated from module0_flow) and preliminary HIP selection code to proto_nd_flow. Also implemented are time-dependent disabled_channels resource and particle_data resource in proto_nd_flow.

Kathryn Sutton and others added 30 commits August 29, 2023 18:51
Copy link
Member

@krwood krwood left a comment

Choose a reason for hiding this comment

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

Thanks Elise! Most of my comments are organizational and are just for you consideration. The major one that would be good to address before merging this PR is the one about the extra datasets in ndlar_flow/src/proto_nd_flow/reco/charge/calib_prompt_hits.py, as that is code that we are using in the mainline 2x2 analysis chain.

@@ -0,0 +1,46 @@
################################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a better place for this to live would be in ndlar_flow/src/proto_nd_flow/util?

@@ -0,0 +1,57 @@
################################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a better place for this to live would be in ndlar_flow/src/proto_nd_flow/util?

@@ -60,6 +60,8 @@ class CalibHitBuilder(H5FlowStage):
ts_pps f8, PPS packet timestamp [ns]
io_group u8, io group ID (PACMAN number)
io_channel u8, io channel ID (related to PACMAN number & PACMAN UART Number)
chip_id u8, chip ID (ASIC number on PACMAN UART)
Copy link
Member

Choose a reason for hiding this comment

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

I know you're using the chip_id and channel_id fields to debug without having to go through the references to packets, but can we remove these from the PR?

@@ -0,0 +1,757 @@
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a better place for this to live would be in ndlar_flow/src/proto_nd_flow/reco/combined?

@@ -0,0 +1,545 @@
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a better place for this to live would be in ndlar_flow/src/proto_nd_flow/reco/combined?

@@ -0,0 +1,26 @@
classname: TrackletMerger
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a better place for this to live would be in ndlar_flow/yamls/proto_nd_flow/reco/combined?

@@ -0,0 +1,25 @@
classname: TrackletReconstruction
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a better place for this to live would be in ndlar_flow/yamls/proto_nd_flow/reco/combined?

Comment on lines +19 to +20
merge_cut: 65 # merge hits with delta t < merge_cut [CRS ticks]
max_merge_steps: 50 # max number of iterations when merging
Copy link
Member

Choose a reason for hiding this comment

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

These seem like they are copied over from CalibHitMerger.yaml - do you need them here or can they be safely removed (just to keep things clean)?

@@ -0,0 +1,26 @@
# Generates the mid-level event built data for charge data (i.e. hits and
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename this file to be a bit more specific?

@@ -0,0 +1,23 @@
# Generates higher-level event built data for charge data (i.e. tracklets)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a better place for this to live would be in ndlar_flow/yamls/proto_nd_flow/reco/combined?

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.

2 participants