Skip to content

Conversation

@mattleblanc
Copy link
Contributor

Hello,

At the moment, the ROOT::TLorentzVector class uses a pseudorapidity-based calculation for the commonly-used DeltaR distance between vectors. For massive objects, such as jets, the rapidity should often be used instead. I have implemented a small switch within the TLorentzVector::DeltaR function to allow users to make this calculation with the vector's rapidity, instead of pseudorapidity. This has a default value of kFALSE, which means that the current behaviour will be the default one.

I have cross-checked this new functionality with the fastjet::pseudojet::delta_R implementation and an internal ATLAS one, which all give the same results when performing the rapidity-based calculation.

Please let me know if I have violated any contribution guidelines, and I will update this PR accordingly!

🍻 Matt

@mattleblanc mattleblanc requested a review from lmoneta as a code owner January 25, 2021 13:23
@phsft-bot
Copy link

Can one of the admins verify this patch?

@mattleblanc
Copy link
Contributor Author

Hi, I guess this should also be implemented in VectorUtil -- this is a work-in-progress until I push another commit. Sorry for not sending it all at once!

MLB

@mattleblanc mattleblanc changed the title Implement rapidity-based DeltaR calculations in TLorentzVector WIP: Implement rapidity-based DeltaR calculations in TLorentzVector Jan 25, 2021
@lmoneta
Copy link
Member

lmoneta commented Jan 25, 2021

Hi Matt

Than you for your contribution!

If you have time, it would be nice to add also this also for the Genvector package for the ROOT::Math::LorentzVector classes. This can be added as a new function in the VectorUtil file, something like DeltaRapidityPhi.
I am not sure if it is better just having just the new function, DeltaRapidityPhi instead of DeltaR with a boolean parameter.

Cheers

Lorenzo

@mattleblanc mattleblanc changed the title WIP: Implement rapidity-based DeltaR calculations in TLorentzVector Implement rapidity-based DeltaR calculations in TLorentzVector Jan 26, 2021
@mattleblanc
Copy link
Contributor Author

mattleblanc commented Jan 26, 2021

I've added this functionality now also to ROOT::Math::GenVector, it gives the same results as the other implementation and fastjet::pseudojet. I'm not sure if this is exactly how you wanted it implemented, so please let me know if you have further comments -- I've tried to keep everything as consistent as possible between the two sets of changes.

I have un-marked the PR as a work-in-progress.

@mattleblanc
Copy link
Contributor Author

Hi @lmoneta, this is just a ping in case you didn't see the other message -- there is obviously no rush, though.

@oshadura
Copy link
Collaborator

@phsft-bot build!

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-fedora31/noimt.
See console output.

@phsft-bot
Copy link

Build failed on ROOT-performance-centos8-multicore/default.
Running on null:/data/sftnight/workspace/root-pullrequests-build
See console output.

Errors:

  • [2021-01-29T11:25:39.269Z] 968/2116 Test cling PR 144 #675: tutorial-math-mathcoreGenVector ...................................................................***Failed Error regular expression found in output. Regex=[: error:] 1.89 sec

Failing tests:

@phsft-bot
Copy link

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2021-01-29T11:34:33.103Z] 816/2176 Test cling PR 162 #702: tutorial-math-mathcoreGenVector ...................................................................***Failed Error regular expression found in output. Regex=[: error:] 0.69 sec

Failing tests:

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

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

In the case of VectorUtil we need two different functions:
DeltaR( v1,v2) for Eta
DeltaRapidityPhi (v1,v2) for Rapidity.

The reason is that we can call DeltaR also for 3D objects and if it is implemented in term of rapidity , it will not compile. You can see this from the failing error from the integration builds.

Can you please add this change ?

Thank you for this contribution !
Lorenzo

@mattleblanc
Copy link
Contributor Author

Hi @lmoneta, that makes sense -- I have just pushed some more changes. Thanks for the review!

MLB

@mattleblanc mattleblanc requested a review from lmoneta February 2, 2021 18:57
@lmoneta
Copy link
Member

lmoneta commented Feb 4, 2021

Thank you for the changes. They look good now.
It would be nice to add also a test for the new DeltaRapidityPhi function, for example adding few lines of code in
https://github.com/root-project/root/blob/master/tutorials/math/mathcoreGenVector.C#L424

If you don't have time doing this, I can do it later, after merging this PR.

@lmoneta
Copy link
Member

lmoneta commented Feb 4, 2021

@phsft-bot build!

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@mattleblanc
Copy link
Contributor Author

Hi @lmoneta, I don't have time to implement this test at the moment, but I will add it to my list to revisit in the future, in case I get there before you do. Thanks once again for reviewing this PR!

@phsft-bot
Copy link

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

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

A test will then be added as a separate PR. Thank you @mattleblanc for this contribution!

@lmoneta lmoneta merged commit f6e6060 into root-project:master Feb 5, 2021
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.

4 participants