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/rename particle functions #263

Merged
merged 21 commits into from
Aug 7, 2024
Merged

Conversation

nilssass
Copy link
Collaborator

@nilssass nilssass commented Jul 30, 2024

In this PR I do two things:

1. Renaming Member Functions

In Particle we have chosen function names that are suboptimal and not intuitive to find or they contained parts in their name that do not contain any useful information. I have renamed them to make them more precise and easier to find. They are:

Old Function Name New Function Name
momentum_rapidity_Y() rapidity()
spatial_rapidity() spacetime_rapidity()
spatial_rapidity_cut() spacetime_rapidity_cut()
pt_abs() pT_abs()
pt_cut() pT_cut()
compute_mass_from_energy_momentum() mass_from_energy_momentum()
compute_charge_from_pdg() charge_from_pdg()

2. Introducing Helper Functions

In tests/test_JetAnalysis.py we were comparing two nested lists using

# Compare the two contents
assert test_jet_finding_content == generated_jet_finding_content

However, this fails on different machines because of numerical precision when comparing floats. I have introduced a new file tests/HelperFunctions.py that contains a function to compare two lists taking into account the limits of numerical precision. This function is now used

@nilssass nilssass requested a review from NGoetz July 30, 2024 12:03
Copy link
Member

@NGoetz NGoetz left a comment

Choose a reason for hiding this comment

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

I think some of the functionality in HelperFunctions could be done using pytest functionalities. Also, the CHANGELOG entry is missing.

tests/HelperFunctions.py Outdated Show resolved Hide resolved
@Hendrik1704
Copy link
Collaborator

Hendrik1704 commented Jul 30, 2024

We should be careful with changing the name of functions in Particle.

  • Have you checked that you changed all occurrences in the codebase to the new names?
  • This is a step that might make some analysis scripts of users incompatible with the new upcoming version. We have to highlight this change in the CHANGELOG.
  • Since we plan to jump to version 2.0.0 next time, this backward incompatibility is already reflected in the version number.

These are just some thoughts coming to my mind after seeing the comment of your PR. Haven't had time to look at the code and the new names yet.

@nilssass
Copy link
Collaborator Author

Absolutely, the Changelog needs of course to be adjusted, this I will add.
Besides that I have carefully checked for all occurrences of the functions in the entire code base and made sure that all tests pass.
I see the point that renaming function names will potentially break user scripts. That's why I want to clean them up all together once, especially for version 2.0, where a user might expect some bigger changes.
With this PR we should be done with name changes and all names are such that they are easily findable if an user works with an IDE.

@nilssass
Copy link
Collaborator Author

@NGoetz this PR is ready for a second review

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

nilssass commented Aug 1, 2024

@Hendrik1704 @NGoetz Once Niklas reviewed the PR, please, do not yet merge it. I am just modifying the open PR #264 as I realized how many code duplications we have in Filters.py. I am just cleaning it up and I think it will be more efficient to first merge #264 once it is ready

Copy link
Member

@NGoetz NGoetz left a comment

Choose a reason for hiding this comment

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

Looks good

@nilssass nilssass force-pushed the sass/rename_particle_functions branch from 798dc41 to 6445283 Compare August 7, 2024 15:47
@nilssass nilssass merged commit 69412dc into sparkx_devel Aug 7, 2024
2 checks passed
@nilssass nilssass deleted the sass/rename_particle_functions branch August 7, 2024 16:27
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.

3 participants