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 GenKt not recognized as pp algorithm #119

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

m-fila
Copy link
Member

@m-fila m-fila commented Jan 31, 2025

GenKt wasn't recognized as a pp algorithm and jet_reconstruction was failing to resolve which algorithm function to use in that case.
The tests for GenKt seemed to be missing

Reference file jet-collections-fastjet-inclusive-genkt-p1.5.json.gz was using different capitalization than files for the algorithms. I couldn't find anything relaying on the filename being spelled this way so renamed for consistency

There is also do_test_compare_to_fastjet function that seems to be unused in the current tests and is probably a relic from #73

function do_test_compare_to_fastjet(strategy::RecoStrategy.Strategy, fastjet_jets;
Let me know if we could remove it

@m-fila m-fila added the enhancement New feature or request label Jan 31, 2025
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.86%. Comparing base (164c6d2) to head (6196918).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/ClusterSequence.jl 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
+ Coverage   73.49%   73.86%   +0.36%     
==========================================
  Files          18       18              
  Lines        1249     1251       +2     
==========================================
+ Hits          918      924       +6     
+ Misses        331      327       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@m-fila
Copy link
Member Author

m-fila commented Jan 31, 2025

Currently GenKt can't be also used with exclusive_jets and n_exclusive_jets. I think there should be a check for power > 0 if GenKt is used, right?

# Check that an algorithm was used that makes sense for exclusive jets
if !(clusterseq.algorithm
(JetAlgorithm.CA, JetAlgorithm.Kt, JetAlgorithm.EEKt, JetAlgorithm.Durham))
throw(ArgumentError("Algorithm used is not suitable for exclusive jets ($(clusterseq.algorithm))"))
end

# Check that an algorithm was used that makes sense for exclusive jets
if !(clusterseq.algorithm
(JetAlgorithm.CA, JetAlgorithm.Kt, JetAlgorithm.EEKt, JetAlgorithm.Durham))
throw(ArgumentError("Algorithm used is not suitable for exclusive jets ($(clusterseq.algorithm))"))
end

@graeme-a-stewart
Copy link
Member

Hello @m-fila

Thanks for this - when I generalised the test setup for e+e- in #73 the GenKt test got lost!

Also, the do_test_compare_to_fastjet is indeed and anachronism now, so it can be deleted.

@graeme-a-stewart
Copy link
Member

Currently GenKt can't be also used with exclusive_jets and n_exclusive_jets. I think there should be a check for power > 0 if GenKt is used, right?

Yes, the merging has to favour small energy depositions first for the exclusive jets selection to make sense. So it should be valid for GenKt with a power >= 0.

Copy link
Member

@graeme-a-stewart graeme-a-stewart left a comment

Choose a reason for hiding this comment

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

Just a small additional improvement suggested

test/test-pp-reconstruction.jl Outdated Show resolved Hide resolved
test/test-pp-reconstruction.jl Outdated Show resolved Hide resolved
src/ClusterSequence.jl Outdated Show resolved Hide resolved
src/ClusterSequence.jl Outdated Show resolved Hide resolved
src/ClusterSequence.jl Outdated Show resolved Hide resolved
src/ClusterSequence.jl Outdated Show resolved Hide resolved
@graeme-a-stewart
Copy link
Member

Hi @m-fila - there's a logic flaw now that prevents GenKt with p >= 0 from running exclusive selections.

I also realised that the same argument applied to EEKt - it should be tested for p >= 0.

So the logic should be more like:

    # Check that an algorithm was used that makes sense for exclusive jets
    if (clusterseq.algorithm  (JetAlgorithm.GenKt, JetAlgorithm.EEKt)) && clusterseq.power < 0
        throw(ArgumentError("Algorithm $(clusterseq.algorithm) requires power >= 0 (power=$(clusterseq.power))"))
    elseif clusterseq.algorithm 
           (JetAlgorithm.CA, JetAlgorithm.Kt, JetAlgorithm.Durham, JetAlgorithm.GenKt, JetAlgorithm.EEKt)
        throw(ArgumentError("Algorithm used is not suitable for exclusive jets ($(clusterseq.algorithm))"))
    end

@graeme-a-stewart
Copy link
Member

It might be useful to write a few tests against the wrong setup, checking exceptions are thrown correctly (would help codecov!).

@m-fila
Copy link
Member Author

m-fila commented Feb 5, 2025

Thanks, fixed message and logic. Added standalone file with tests - I didn't like splitting them and putting in existing pp and ee tests

@graeme-a-stewart graeme-a-stewart added this to the 0.4.4 Release milestone Feb 5, 2025
Copy link
Member

@graeme-a-stewart graeme-a-stewart left a comment

Choose a reason for hiding this comment

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

Just one tiny little snag - the comment at the top of the test-selection file. Otherwise LGTM!

@@ -0,0 +1,46 @@
# File: test/test_test-selection.jl
Copy link
Member

Choose a reason for hiding this comment

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

Filename is wrong.

The comment should describe better the aim of these tests of exclusive jet selection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants