-
Notifications
You must be signed in to change notification settings - Fork 0
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
Sass/add mT to particle #264
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good in general. One thought that came up is that it might be useful to also implement a filter for mT in the filters for Oscar and Jetscape. If you want to generate only a mT spectrum, then this could save a lot of memory. Maybe it's also useful in other cases.
Thank you for the comment @Hendrik1704 . I will let you know as soon as the PR is ready for the second review |
@Hendrik1704 this PR is ready for a second review.
Besides that, the warning behavior of some cuts were not consistent in case of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few typos in some comments and also some copy-paste mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the new nan outputs of the Particle functions captured in the unit tests?
Yes, I have just added them. Once the tests are complete the PR is ready for the final review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good now. Thanks!
Great, thank you for capturing the typos :D |
This PR adds transverse mass as a member function to Particle. Besides the general use case, we need the transverse mass for the new BulkObservables class.