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

Fix the "fraction" definition #138

Merged
merged 3 commits into from
Jun 11, 2023
Merged

Fix the "fraction" definition #138

merged 3 commits into from
Jun 11, 2023

Conversation

drinkingkazu
Copy link
Contributor

This addresses the issue #137

@soleti
Copy link
Collaborator

soleti commented May 21, 2023

I think this can safely be merged, but maybe one extra person should independently verify this. @krwood?
@drinkingkazu, just to double check, you want to add also the 2x2 notebook?

@krwood
Copy link
Member

krwood commented May 22, 2023

@YifanC is going to handle the testing and approving of this PR. IMO one thing that might be nice to see is summed fractional contributions from a sufficiently complex event for (1) before this PR with MAX_TRACKS_PER_PIXEL set high vs. (2) before this PR with MAX_TRACKS_PER_PIXEL set low vs. (3) after this PR with MAX_TRACKS_PER_PIXEL set high vs. (2) after this PR with MAX_TRACKS_PER_PIXEL set low.

I do agree with @drinkingkazu and @soleti that this seems like a nice improvement. We may even want to put in an error or warning message that is triggered when the summed fractional contributions fall short of 100%?

@soleti soleti requested a review from YifanC May 23, 2023 09:49
@krwood krwood mentioned this pull request Jun 10, 2023
@krwood
Copy link
Member

krwood commented Jun 10, 2023

Pull Request #140 is very similar to this one, but it also implements a fractional contribution normalization that is in essence q_sum but without the noise superimposed on the signal. I suggest we merge #140 and close this one, but please shout if you disagree @drinkingkazu.

@YifanC YifanC merged commit d78d364 into DUNE:develop Jun 11, 2023
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.

5 participants