Updated CommonTools/RecoUtils also for 7XY #3928
Conversation
|
A new Pull Request was created by @mageisler (Matthias Geisler) for CMSSW_7_0_X. Updated CommonTools/RecoUtils also for 7XY It involves the following packages: CommonTools/RecoUtils @nclopezo, @vadler, @cmsbuild, @Degano, @monttj can you please review it and eventually sign? Thanks. |
|
@mageisler : Since this is not mergable in 71X and cannot (and must not!) be forwarded due to the prefetching -- AKA |
| @@ -945,81 +1023,247 @@ PF_PU_AssoMapAlgos::FindAssociation(const reco::TrackRef& trackref, std::vector< | |||
| /*************************************************************************************/ | |||
|
|
|||
| int | |||
| PF_PU_AssoMapAlgos::DefineQuality(int assoc_ite, int step, double distance) | |||
| PF_PU_AssoMapAlgos::DefineQuality(vector< pair<int, double> > distances, int step, double distance) | |||
There was a problem hiding this comment.
It looks like there is some code duplication in this function in the various conditionals/switches, no?
There was a problem hiding this comment.
I hope not, I know the definitions get messy for case 2 (the final association), but there are three options to cover and different cut values for each option depending on the previous associations. Hence, in total about 6 possibilities with several quality definitions need to be set... and I hope I didn't get lost in defining them
There was a problem hiding this comment.
I did not mean the definition of the cases, but the code inside. I looks like there was a copy&paste exercise, which I would prefer to be encapsulated in a function, if possible.
There was a problem hiding this comment.
aah, ok, I am not sure, could be... maybe it can be encapsulated, but its hard since all definitions depend on at least four parameters, and also this number of parameters is not always the same
There was a problem hiding this comment.
Ah, got it now: there are these slight differences in the usage of f1, fz and f3. It's a bit hard to see ;-)
|
The test configs should use some commonly availabel inputs, like e.g. RelVals and a proper GT (still the 62X one here), but these are no show stoppers and can be fixed later. |
|
Ok for me. I will wait with the signature until the 71X version is available. |
|
... or rather 72X first! |
|
@mageisler |
|
Hi, sorry, if I remember correctly I already send a pull request for 71X. And actually, after sending a pull request for 62X, 70X and 71X, I thought its a joke that I need to send also a pull request for 72X. I will work on that tomorrow, Am 30.06.2014 um 22:02 schrieb Tae Jeong Kim <notifications@github.commailto:notifications@github.com>: @mageislerhttps://github.com/mageisler — |
|
@mageisler |
|
@Tae: The old pull request for 71X was #3920. It has been closed but the final comment has been that automatic forwarding from 70X to 71X is on. I think I will just (re)submit the pull request for 71X and 72X. Am 30.06.2014 um 22:24 schrieb Tae Jeong Kim <notifications@github.commailto:notifications@github.com>: @mageislerhttps://github.com/mageisler — |
|
+1 for now for 70X |
|
+1 |
Updated CommonTools/RecoUtils also for 7XY
Pull request for an update for CommonTools/RecoUtils for CMSSW 70X. Its basically the same as request 3259 was for 62Y.