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

Jet substructure structs - do we need them? #136

Closed
graeme-a-stewart opened this issue Mar 18, 2025 · 3 comments · Fixed by #138
Closed

Jet substructure structs - do we need them? #136

graeme-a-stewart opened this issue Mar 18, 2025 · 3 comments · Fixed by #138
Assignees
Labels
interface change Breaking changes to any public interfaces question Further information is requested
Milestone

Comments

@graeme-a-stewart
Copy link
Member

Discussing with @m-fila he pointed out that the current scheme of calling mass_drop, soft_drop, jet_filtering and jet_trimming with special structs is sub-optimal. These structs simply gather together a few parameters and pass them into the function. There's no great value in having them as structs, it seems a bit overkill for functions with a few parameters.

A better option is to convert the functions to take named parameters, e.g.,

mass_drop(jet::PseudoJet, clusterseq::ClusterSequence; mu, y)

If the user wants to gather the parameters together, for consistency, then this can be done as Julia named tuple and an argument splat:

md_args = (mu=0.67, y=0.09)
mass_drop(jet, clusterseq; md_args...)

For soft drop, where there is a sensible default for the cluster radius then this can have the normal argument default:

function soft_drop(jet::PseudoJet, clusterseq::ClusterSequence; zcut, beta, radius=1.0)

N.B. I would propose that we rename the second parameter beta (β is probably a bit too far...).

Thoughts on this @m-fila @sattwamo?

@graeme-a-stewart graeme-a-stewart added interface change Breaking changes to any public interfaces question Further information is requested labels Mar 18, 2025
@graeme-a-stewart graeme-a-stewart added this to the 0.5.0 Release milestone Mar 18, 2025
@m-fila
Copy link
Member

m-fila commented Mar 18, 2025

Just for bookkeeping, I'm not sure what is the benefit in keeping them as structs. One thing I can think of is that maybe a struct based looks more similar to FastJet?
To me having them as keywords arguments is cleaner and more natural while the usability stays the same.

N.B. I would propose that we rename the second parameter beta (β is probably a bit too far...).

Probably I'm not fully converted yet but I don't like the idea of having fancy glyphs as keyword arguments. But beta is fine

I'm not sure how far away is 0.5.0 release, but probably it's possible even now to add functions with keyword arguments and deprecate the functions with structs?

@sattwamo
Copy link
Collaborator

Just as @m-fila stated, I had used the structs to make an interface similar to FastJet. But I agree that using named parameters might be better in this case. I can make the necessary changes and create a new PR.

@graeme-a-stewart
Copy link
Member Author

Yes, @m-fila - this is exactly what I thought. And the trick of splatting a named tuple for named parameters is very idiomatic Julia.

Thanks for agreeing to handle this, @sattwamo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interface change Breaking changes to any public interfaces question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants