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

feat: adding groomed jet radius and momentum balance variables for softdrop groomed jet #312

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kawaho
Copy link

@kawaho kawaho commented Sep 11, 2024

resolves #303

@kawaho kawaho changed the title adding groomed jet radius and momentum balance variables for softdrop groomed jet feat: adding groomed jet radius and momentum balance variables for softdrop groomed jet Sep 11, 2024
@matthewfeickert
Copy link
Member

matthewfeickert commented Sep 11, 2024

@kawaho In the future please do not make PRs from your main branch and always use a feature branch. c.f. https://hynek.me/articles/pull-requests-branch/ Using your main branch for PRs makes it impossible to rebase your branch against the upstream without closing the PR.

Copy link
Collaborator

@rkansal47 rkansal47 left a comment

Choose a reason for hiding this comment

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

Looks pretty good @kawaho!

Unfortunately there is a runtime error with the function now https://github.com/scikit-hep/fastjet/actions/runs/10819914062/job/30018826074?pr=312#step:7:135:

self = <fastjet._singleevent._classsingleevent object at 0x116f66a90>, njets = 1
beta = 0.0, symmetry_cut = 0.1, symmetry_measure = 'scalar_z', R0 = 0.8
recursion_choice = 'larger_pt', mu_cut = inf

    def exclusive_jets_softdrop_grooming(
        self,
        njets=1,
        beta=0.0,
        symmetry_cut=0.1,
        symmetry_measure="scalar_z",
        R0=0.8,
        recursion_choice="larger_pt",
        # subtractor = 0,
        mu_cut=float("inf"),
    ):
        # if njets <= 0:
        #    raise ValueError("Njets cannot be <= 0")
>       np_results = self._results.to_numpy_softdrop_grooming(
            njets,
            beta,
            symmetry_cut,
            symmetry_measure,
            R0,
            recursion_choice,  # subtractor,
            mu_cut,
        )
E       RuntimeError: std::bad_cast

../../../hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/fastjet/_singleevent.py:215: RuntimeError

Perhaps the types for the new variables is incorrect?

Also, can you update the test_exclusive_jets_softdrop_grooming function as well please.

@kawaho kawaho marked this pull request as draft September 12, 2024 02:24
@kawaho
Copy link
Author

kawaho commented Sep 12, 2024

@rkansal47 I think the types for the new variables are fine but the error comes from the use of dynamic_cast in https://github.com/alisw/fastjet/blob/master/fastjet/include/fastjet/PseudoJet.hh#L1126 . It is also strange because the error only happens for macOS, not sure how to resolve it yet....

@kawaho
Copy link
Author

kawaho commented Sep 12, 2024

@rkansal47 I think the types for the new variables are fine but the error comes from the use of dynamic_cast in https://github.com/alisw/fastjet/blob/master/fastjet/include/fastjet/PseudoJet.hh#L1126

I have done more tests, separating out the soft.structure_of<fastjet::contrib::SoftDrop>() call, and can confirm the RuntimeError: std::bad_cast is from the dynamic_cast. But not sure how to fix yet.

@matthewfeickert
Copy link
Member

@kawaho I've updated this PR to bring in the state of the upstream HEAD, so if you pull your branch will now have the changes from PR #311 in it which should make local development easier. To have a fully clean repository you'll still need to do the git clean steps of Issue #279, but you should be able to do new builds by just running

python -m pip install --upgrade .

without having to run the cleaning steps from Issue #279. If you have questions just ask here. 👍

@kawaho
Copy link
Author

kawaho commented Sep 13, 2024

@matthewfeickert Thanks for the PR, it works great.

@@ -1703,6 +1705,11 @@ PYBIND11_MODULE(_ext, m) {
jet_groomed_m.push_back(soft.m());
jet_groomed_E.push_back(soft.E());
jet_groomed_pz.push_back(soft.pz());

auto sd_struct = soft.structure_of<fastjet::contrib::SoftDrop>();
Copy link
Author

Choose a reason for hiding this comment

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

@matthewfeickert and @rkansal47 I would like to hear suggestions on solving this failed test (https://github.com/scikit-hep/fastjet/actions/runs/10819914062/job/30018826074?pr=312#step:7:135). The RuntimeError std::bad_cast is coming from this specific line calling the dynamic casting in (https://github.com/alisw/fastjet/blob/master/fastjet/include/fastjet/PseudoJet.hh#L1126). The error thrown is happening on MacOS only. I can replicate this error on my own Mac but calling the same function in C++ only script with the same fastjet-contrib version did not give such an error. So I suspect this is related to pybind or how we compile the code. I am unfamiliar with both and would like to hear suggestions for things to try from any experts on this project.

Copy link
Member

Choose a reason for hiding this comment

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

Probably want to ask (in addition to @rkansal47) other people that have touched the C++: @jpivarski @jmduarte @chrispap95 @lgray @aryan26roy

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.

Adding groomed jet radius and momentum balance for the soft drop groomed jet
3 participants