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
LoKi functors lesson updated and expanded #26
Conversation
Lesson updated, couple of typos and mistakes corrected. Also, several new useful things added. Among them: - several useful functors - note on the unit conversion (MeV->GeV) - several new challenges which may be interesting both from the implementation and physics point of view (D0 vertex quality, kaons track quality, ...), and will avoid bored students from falling asleep - proposal to check and understand the LoKi functors used in the stripping line itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a really nice set of improvements, just a few minor changes to make 👍
@@ -64,6 +65,15 @@ harmless in the examples we will use. | |||
If the import is made *before* the instantiation of the `ApplicationMgr`, there | |||
will be no warnings. | |||
|
|||
{% callout "A note about units" %} | |||
By convention, the LHCb default units are MeV, centimeters and picoseconds. It is easy to print the values of interest in other units: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the convention for time was nanoseconds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it's my fault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem 😄 also I've just realised it should be millimeters instead of centimeters
```python | ||
from GaudiPython.Bindings import AppMgr, gbl | ||
gaudi = AppMgr() | ||
distCal = gaudi.toolSvc().create("LoKi::DistanceCalculator", interface = gbl.IDistanceCalculator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the python style guide:
Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value.
@@ -181,7 +211,7 @@ from LoKiPhys.decorators import MAXTREE, ISBASIC, HASTRACK | |||
MAXTREE(ISBASIC & HASTRACK, PT, -1)(cand) == max_pt | |||
``` | |||
|
|||
In this example, we have used two selection functors, `ISBASIC` and `HASTRACK`, which return true if the particle doesn't have children and is made up by a track, respectively. | |||
In this example, we have used two selection functors, `ISBASIC` and `HASTRACK`, which return true if the particle doesn't have children and is made up by a track, respectively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've gained a space at the end of this line.
functor returned. | ||
Retrieve the momentum magnitude using functors `PX`, `PY` and `PZ`. | ||
|
||
There is also a specific functor `P` which does the job. Compare the results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of this was to use the momentum()
particle function, not to compare functors. This could have been made clearer, but it's important to establish this parallelism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah well, now I see. Just didn't get the point when reading the old version. Will modify to make it clear.
- Comments from Chris and Albert taken into account - The introductory part changed in order to reflect better what Albert was pointing out, that required some permutations in order to be consistent. Please check if what's written now makes sense.
Any comments on the updated version of the lesson? Just wonder whether all the things I changed are correct from the 'technical' or language point of view. |
Everything looks great to me, thank you for the improvements! 👍 I'll just give @apuignav a chance to comment it is merged. |
I'm happy too, thanks a lot! Merging! |
Thank you. |
Mm, yes. I know about this and need to fix it. |
Lesson updated, couple of typos and mistakes corrected. Also, several new useful things added.
Among them: