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 multiplicity_cut() function #296

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Conversation

LucasConstantin
Copy link
Collaborator

This closes issue #294. Oscar.multiplicity_cut() now takes a tuple and the update_num_output_per_event_after_filter()' now correctly updates the new num_output_per_event. I also add the case that the number of events is zero in the print_particle_lists_to_file()` function.

@LucasConstantin LucasConstantin changed the base branch from main to sparkx_devel October 24, 2024 14:35
@NGoetz NGoetz requested a review from nilssass October 24, 2024 14:42
@NGoetz
Copy link
Member

NGoetz commented Oct 24, 2024

The tests are failing, apparently the changes broke things. I will review once they pass.

@NGoetz
Copy link
Member

NGoetz commented Oct 24, 2024

In general it is advisable to actually run the tests before pushing.

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.

While your changes generally fix the underlying issue, I think you should work a bit more carefully and also try to understand the logic of the codebase you are editing.

src/sparkx/Oscar.py Show resolved Hide resolved
tests/test_Filter.py Show resolved Hide resolved
src/sparkx/Filter.py Outdated Show resolved Hide resolved
@NGoetz
Copy link
Member

NGoetz commented Nov 6, 2024

@LucasConstantin please take a look at the required changes.

@LucasConstantin LucasConstantin force-pushed the constantin/fix_multiplicity_cut branch from 79bba84 to 799fa39 Compare November 7, 2024 14:21
@NGoetz
Copy link
Member

NGoetz commented Nov 7, 2024

@Hendrik1704 do you understand why the checks are not running for this PR? Could it be that checks only run for pushes and not force-pushes?

@Hendrik1704
Copy link
Collaborator

@NGoetz According to ChatGPT any kind of push triggers the actions. Maybe it's because there are conflicts.

@NGoetz
Copy link
Member

NGoetz commented Nov 7, 2024

@LucasConstantin can you please try to rebase on sparkx_devel tomorrow or on Monday? Reviewing only makes sense once the Actions succeed.

Lucas Constantin added 2 commits November 11, 2024 12:54
multiplcity_cut() function now takes a tuple for
the upper and lower bounds of the multiplicity multiplcity_cut
Also fixes issues for cuts that decrease the number of events where
num_output_per_event_ was not properly updated.
Fix num_output_per_event() so that it starts counting from 0 for Oscar
and from 1 for Jetscape files. Also add test for warning in case of
empty event list.
@LucasConstantin LucasConstantin force-pushed the constantin/fix_multiplicity_cut branch from 799fa39 to acd442a Compare November 11, 2024 12:11
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.

Very good, and congrats for your first successful rebase! I have a minor error-handling request. On the top you can see the reason for your action fail:
src/sparkx/BaseStorer.py:851: error: Need type annotation for "updated_num_output_per_event" [var-annotated]
You forgot the type of updated_num_output_per_event

src/sparkx/Oscar.py Show resolved Hide resolved
tests/test_Oscar.py Show resolved Hide resolved
@Hendrik1704
Copy link
Collaborator

@LucasConstantin The static typing action failed for this function: updated_num_output_per_event in BaseStorer
Please check if all the data types are given there.

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, thank you!

@NGoetz NGoetz merged commit cb5a3b8 into sparkx_devel Nov 18, 2024
2 checks passed
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