Fix plotting for ClusterSequence and update serialisation
#46
Conversation
src/Serialize.jl
Outdated
| "py" => JetReconstruction.py, | ||
| "pz" => JetReconstruction.pz, | ||
| "phi" => JetReconstruction.phi, | ||
| "m" => JetReconstruction.mass, | ||
| "eta" => JetReconstruction.eta | ||
| "eta" => JetReconstruction.rapidity, |
There was a problem hiding this comment.
should "eta" => JetReconstruction.eta?,
There was a problem hiding this comment.
well when I first wrote that my idea was that we demand from the user to define eta() for their type. right now, according to the README.md, only pt2(), phi(), rapidity(), px(), py(), pz(), energy() are required
There was a problem hiding this comment.
and as far as I remember, eta is rapidity (I could be wrong here though)
There was a problem hiding this comment.
eta is pseudorapidity, the two are only strictly equal when the object has mass = 0.
For a "jet", the mass is usually not negligible.
There was a problem hiding this comment.
doesn't the rapidity actually give the same thing in the code? I honestly don't remember these details already
src/Serialize.jl
Outdated
| "pt" => JetReconstruction.pt, | ||
| "kt" => JetReconstruction.pt, | ||
| "pt2" => JetReconstruction.pt2, | ||
| "kt" => JetReconstruction.pt2, |
There was a problem hiding this comment.
pt() is no more required to be defined and there's now only pt2. I understand the confusion though
There was a problem hiding this comment.
if it's absurd from the standpoint of physics then we could remove kt from the map entirely
There was a problem hiding this comment.
I thought "kt" is transverse momentum, thus should equal to pt not pt2 (which is sqaure of that?)
There was a problem hiding this comment.
well, as I've said I forgot how this works because I haven't been doing this physics in a while :)
I think addressing both of your comments, I could just leave it as it was, since it won't really crash unless someone tries to use "kt" in formatting without defining the kt() method for their type
There was a problem hiding this comment.
nevermind, it actually will crash. I'll just stick to the set of required methods then: pt2(), phi(), rapidity(), px(), py(), pz(), energy()
|
should be alright now |
|
LGTM! |
|
Do we wanna do like a "round trip" as unit test? Or that doesn't make much sense |
what do you mean? that we put data loading -> algo execution -> plotting -> result saving in a unit test? |
|
I just mean that |
|
oh, that could make sense, but we could do that in a separate PR |
|
I think we're good to go with this one |
This PR tackles the issue #43 by implementing an additional variant of the
jetsplotfunction that works withClusterSequenceon top of the existing old one that has been developed in 2022 at the beginning of the project. Additionally, I have updated the serialisation methods, as they were partially deprecated andsavejetswas incompatible with the new things in the repository.