Skip to content

Remove nearest neighbor hack - #173

Open
parshadkp wants to merge 18 commits into
cositools:develop/emfrom
parshadkp:NN_strip_fix
Open

Remove nearest neighbor hack#173
parshadkp wants to merge 18 commits into
cositools:develop/emfrom
parshadkp:NN_strip_fix

Conversation

@parshadkp

Copy link
Copy Markdown

This will remove the nearest neighbor hack that was introduced a while ago to calculate the correct dead time in the strip trigger module. The change is simple but I will make this "ready to review" once PR #165 is merged. This will also close Issue #93.

@parshadkp
parshadkp requested a review from fhagemann June 29, 2026 19:48
@parshadkp parshadkp added the DEE Development related to the detector effects engine label Jun 29, 2026
@fhagemann

Copy link
Copy Markdown

This might probably also need the implementation of strip thresholds + some nearest neighbor logic on top of #165, but I can take care of that once #165 is approved.

@parshadkp parshadkp linked an issue Jun 29, 2026 that may be closed by this pull request
@fhagemann

Copy link
Copy Markdown

Now that #165 is merged, we should also tackle this.

@parshadkp
parshadkp marked this pull request as ready for review August 11, 2026 16:47
@fhagemann

Copy link
Copy Markdown

You might want to check if this PR still gives reasonable results with the changes in #190 ;)
If not, let me know, then this might be a bug in #190.

@ckierans ckierans left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, Parshad. I'm fine with you removing the CountRate function, since it seems like something that doesn't quite belong here as a diagnostic, but I just wanted to check your motivation for removing it.

Comment thread include/MSubModuleStripTrigger.h
ASICChannelsSet.insert(ID);
ASICChannelsSet.insert(ID + 1);
}
ASICChannelsSet.insert(ID);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That's much easier :)

@cositools cositools deleted a comment from codecov-commenter Aug 18, 2026
@parshadkp
parshadkp requested a review from ckierans August 24, 2026 22:57
@parshadkp

Copy link
Copy Markdown
Author

Hi @ckierans, I think this looks good to merge now but let me know if you have any other comments.

@fhagemann

Copy link
Copy Markdown

But this also has the code from PR190 in it now?

…fix"

This reverts commit ad70275, reversing
changes made to dc94c0d.
…er_parshad_fork into NN_strip_fix"

This reverts commit 6006d76, reversing
changes made to 0a0bcc3.
@parshadkp

Copy link
Copy Markdown
Author

But this also has the code from PR190 in it now?

Sorry, I must have accidentally pushed my local changes when I was testing. Should be fixed now.

@fhagemann fhagemann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Then let's merge this PR together with #190 once that is ready :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DEE Development related to the detector effects engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove nearest neighbor hack

3 participants