scikit-hep / fastjet

Jet-finding in the Scikit-HEP ecosystem.
https://fastjet.readthedocs.io
BSD 3-Clause "New" or "Revised" License
21 stars 14 forks source link

Fix issue with array size #175

Closed chrispap95 closed 1 year ago

chrispap95 commented 1 year ago

This is a fix for the issue presented in #174. In some parts, the code has statements like:

css[i]->inclusive_jets().size()

to get the size of the individual arrays. However, this means that inclusive_jets() is always called with min_pt=0 and this can generate higher sizes in many cases.

PS. 1: I am afraid there might be other parts in the code with similar issues. I don't have the time to look now but might be worth visiting this again in the future. PS. 2: Unfortunately, the clang hook is not very strict in enforcing formatting. I would like to introduce more consistent tabbing but I refrained from pushing it to this PR to keep its scope clear.

chrispap95 commented 1 year ago

It would be very nice to devise some kind of test for this as well.

chrispap95 commented 1 year ago

Also, it would be great if @jpata could compile from source with this change and confirm it works for him. I run the file & the code snippet he posted in the issue and after this change everything looks okay, but you never know.

Edit: In fact, no compilation should be needed. I believe you should be able to grab the artifacts from the CI check.

chrispap95 commented 1 year ago

Thanks, it's awesome to have a test for this! Out of curiosity, did you check if the test fails with the current release?

jmduarte commented 1 year ago

Yes, it does fail with the current release fastjet 3.4.0.4:

platform darwin -- Python 3.11.0, pytest-7.2.1, pluggy-1.0.0 -- /opt/miniconda3/envs/fastjet/bin/python3.11
cachedir: .pytest_cache
rootdir: /Users/jduarte/Dropbox/fastjet, configfile: setup.cfg
collected 13 items                                                                                                              

test_006-misc.py::test_unique_history_multi PASSED                                                                        [  7%]
test_006-misc.py::test_unique_history_single PASSED                                                                       [ 15%]
test_006-misc.py::test_n_particles_single PASSED                                                                          [ 23%]
test_006-misc.py::test_n_particles_multi PASSED                                                                           [ 30%]
test_006-misc.py::test_n_exclusive_jets_multi PASSED                                                                      [ 38%]
test_006-misc.py::test_n_exclusive_jets_single PASSED                                                                     [ 46%]
test_006-misc.py::test_unclustered_particles_multi PASSED                                                                 [ 53%]
test_006-misc.py::test_unclustered_particles_single PASSED                                                                [ 61%]
test_006-misc.py::test_childless_pseudojets_single PASSED                                                                 [ 69%]
test_006-misc.py::test_childless_pseudojets_multi PASSED                                                                  [ 76%]
test_006-misc.py::test_jets_single PASSED                                                                                 [ 84%]
test_006-misc.py::test_jets_multi PASSED                                                                                  [ 92%]
test_006-misc.py::test_constituent_index_min_pt FAILED                                                                    [100%]

=========================================================== FAILURES ============================================================
_________________________________________________ test_constituent_index_min_pt _________________________________________________

    def test_constituent_index_min_pt():
        # test for https://github.com/scikit-hep/fastjet/issues/174
        array = ak.Array(
            [
                [
                    {
                        "px": 0.32786062359809875,
                        "py": -0.8113271594047546,
                        "pz": -0.9425322413444519,
                        "E": 0.1395701766014099,
                    },
                    {
                        "px": 1.2147544622421265,
                        "py": -6.664909839630127,
                        "pz": -0.09962931275367737,
                        "E": 0.1395701766014099,
                    },
                    {
                        "px": 0.031138502061367035,
                        "py": -0.03814707323908806,
                        "pz": -0.19269536435604095,
                        "E": 0.0,
                    },
                    {
                        "px": 0.25616654753685,
                        "py": -2.1868643760681152,
                        "pz": -1.1193745136260986,
                        "E": 0.0,
                    },
                    {
                        "px": 0.1986897736787796,
                        "py": -0.08637325465679169,
                        "pz": 0.16585613787174225,
                        "E": 0.0,
                    },
                    {
                        "px": 7.569584846496582,
                        "py": -8.49626636505127,
                        "pz": 8.851030349731445,
                        "E": 0.9395653009414673,
                    },
                ],
                [
                    {
                        "px": -1.995545744895935,
                        "py": 1.0673128366470337,
                        "pz": -6.9042792320251465,
                        "E": 0.1395701766014099,
                    },
                    {
                        "px": -9.267221450805664,
                        "py": 2.7426843643188477,
                        "pz": -0.42873385548591614,
                        "E": 0.1395701766014099,
                    },
                    {
                        "px": -1.5686458349227905,
                        "py": -0.1490965336561203,
                        "pz": -0.23429453372955322,
                        "E": 0.9395653009414673,
                    },
                    {
                        "px": -1.043339729309082,
                        "py": 0.6251208186149597,
                        "pz": -1.4011896848678589,
                        "E": 0.0,
                    },
                    {
                        "px": 2.5927963256835938,
                        "py": -4.718469142913818,
                        "pz": -5.502776145935059,
                        "E": 0.0,
                    },
                ],
            ]
        )

        jetdef = fastjet.JetDefinition(fastjet.antikt_algorithm, 0.4)
        cluster = fastjet.ClusterSequence(array, jetdef)
        constituent_idx = cluster.constituent_index(min_pt=0.5)

>       assert constituent_idx.to_list() == [[[3], [1], [0, 5]], [[3], [2], [0], [4], [1]]]
E       assert [[[], [], []], [[], [], [], [], []]] == [[[3], [1], [0, 5]], [[3], [2], [0], [4], [1]]]
E         At index 0 diff: [[], [], []] != [[3], [1], [0, 5]]
E         Full diff:
E         - [[[3], [1], [0, 5]], [[3], [2], [0], [4], [1]]]
E         ?    -    -    ----      -    -    -    -    -
E         + [[[], [], []], [[], [], [], [], []]]

test_006-misc.py:347: AssertionError
================================================= 1 failed, 12 passed in 2.07s ==================================================
chrispap95 commented 1 year ago

That's awesome! It's good that we have an easy way to test for these issues because there might be more in the code. I didn't have the time to go over all methods but we should at some point.