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

Check also first element in lumi for is_fonllb #30

Merged
merged 1 commit into from
Jul 14, 2022
Merged

Conversation

andreab1997
Copy link
Contributor

@andreab1997 andreab1997 commented Jul 14, 2022

we want that check.is_fonllb checks also the first entry in each grid.lumi element.
Solve #26

@andreab1997 andreab1997 added the bug Something isn't working label Jul 14, 2022
@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #30 (f919c6d) into main (2dc827f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #30   +/-   ##
=======================================
  Coverage   40.86%   40.86%           
=======================================
  Files          15       15           
  Lines         531      531           
=======================================
  Hits          217      217           
  Misses        314      314           
Flag Coverage Δ
unittests 40.86% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pineko/check.py 66.66% <100.00%> (ø)

@andreab1997 andreab1997 merged commit eccb372 into main Jul 14, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix_check_fonllb branch July 14, 2022 15:58
@@ -52,7 +53,7 @@ def test_contains_ren():

def test_is_fonll_b():
fns = "FONLL-B"
lumi = [[(1, 11, 3, 4), (3, 11, 5, 6)], [(9, 11, 0, 3), (8, 11, -2, -1)]]
lumi = [[(1, 11, 3, 4), (3, 11, 5, 6)], [(9, 11, 0, 3), (-12, 3, -2, -1)]]
Copy link
Member

Choose a reason for hiding this comment

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

Being this an "insane" example I would add instead to sane ones: one in which the first object is a proton, and another one in which it is the second. The one here is simply an invalid luminosity.

@@ -84,7 +84,7 @@ def is_fonll_b(fns, lumi):
"""
for lists in lumi:
for el in lists:
if not (10 < abs(el[1]) < 17):
if (not (10 < abs(el[0]) < 17)) and (not (10 < abs(el[1]) < 17)):
Copy link
Member

Choose a reason for hiding this comment

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

Since I'm writing on this PR in any case: instead of repeating twice the condition, make a brief is_lepton function (even in this function scope, or top level in the file, as you wish).

@alecandido
Copy link
Member

@andreab1997 I know this PR has already been merged, but it was more convenient to write here, since it is next to the code.

@andreab1997
Copy link
Contributor Author

@andreab1997 I know this PR has already been merged, but it was more convenient to write here, since it is next to the code.

Yes, sorry to have merged it without waiting you: I thought it was trivial :)

@andreab1997 andreab1997 restored the fix_check_fonllb branch July 15, 2022 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants