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

Sass/add mT to particle #264

Merged
merged 11 commits into from
Aug 6, 2024
Merged

Sass/add mT to particle #264

merged 11 commits into from
Aug 6, 2024

Conversation

nilssass
Copy link
Collaborator

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.

Copy link
Collaborator

@Hendrik1704 Hendrik1704 left a 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.

@Hendrik1704 Hendrik1704 added this to the SPARKX 2.0 milestone Jul 31, 2024
@nilssass
Copy link
Collaborator Author

nilssass commented Aug 1, 2024

Thank you for the comment @Hendrik1704 . I will let you know as soon as the PR is ready for the second review

@nilssass
Copy link
Collaborator Author

nilssass commented Aug 4, 2024

@Hendrik1704 this PR is ready for a second review.
I have realized that we have quite some code duplications in Filter.py Therefore I have encapsulated some tests of cutting tuples in the new function

__ensure_tuple_is_valid_else_raise_error(value_tuple, allow_none=False)

Besides that, the warning behavior of some cuts were not consistent in case of lower_cut > upper_cut. This PR solves this issue. Now, all cuts will give a warning and automatically switch the order of the cutting values.

@Hendrik1704 Hendrik1704 self-requested a review August 6, 2024 13:07
Copy link
Collaborator

@Hendrik1704 Hendrik1704 left a 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.

src/sparkx/Filter.py Outdated Show resolved Hide resolved
src/sparkx/Filter.py Show resolved Hide resolved
src/sparkx/Filter.py Outdated Show resolved Hide resolved
src/sparkx/Filter.py Outdated Show resolved Hide resolved
src/sparkx/Filter.py Outdated Show resolved Hide resolved
src/sparkx/Jetscape.py Outdated Show resolved Hide resolved
src/sparkx/Oscar.py Outdated Show resolved Hide resolved
src/sparkx/Particle.py Outdated Show resolved Hide resolved
tests/test_Filter.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Hendrik1704 Hendrik1704 left a 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?

@nilssass
Copy link
Collaborator Author

nilssass commented Aug 6, 2024

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

@Hendrik1704 Hendrik1704 self-requested a review August 6, 2024 15:29
Copy link
Collaborator

@Hendrik1704 Hendrik1704 left a 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!

@nilssass
Copy link
Collaborator Author

nilssass commented Aug 6, 2024

Great, thank you for capturing the typos :D

@nilssass nilssass merged commit 020eae8 into sparkx_devel Aug 6, 2024
2 checks passed
@nilssass nilssass deleted the sass/add_mT_to_Particle branch August 6, 2024 15:32
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