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
HGCAL Clue algorithm 106 x #26194
HGCAL Clue algorithm 106 x #26194
Conversation
The code-checks are being triggered in jenkins. |
@rovere |
Done. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26194/8791
|
A new Pull Request was created by @rovere (Marco Rovere) for master. It involves the following packages: DataFormats/EgammaReco @perrotta, @andrius-k, @kmaeshima, @schneiml, @civanch, @kpedro88, @cmsbuild, @jfernan2, @fioriNTU, @slava77, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
The tests are being triggered in jenkins. |
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
+1 |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
Looking at the jenkins automatic comparisons, this seems a net improvement to me, with the unphysical peak at cluster energy = 1 now disappeared and spread over lower values (wf20034, TTbar14TeV2023D17) Overall, there are now significantly more clusters, superclusters, etc., with some peculiar geometrical structure: |
I notice some possible issue with validation, where there are plots now empty, see e.g.: Could you please verify what's going on there? |
Ciao @perrotta thanks for looking into this. From a quick look at the plots I see that the "empy" ones are related mainly to duplicates, fakes and distancetoseedcell. This is expected due to the better performance on CLUE. In the old implementation a single fluctuation of noise would have resulted in a cluster, most likely either a fake or a duplicate. Those now, are gone for good. |
Density getDensity(); | ||
Density getDensity() override; | ||
|
||
static void fillPSetDescription(edm::ParameterSetDescription& iDesc) { |
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.
This PSet description is the same as the one above for HGCalCLUEAlgo (a part the different numerical values for "deltac"): shouldn't this method be included in the base class, instead?
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.
@perrotta this is true for the moment. There is work ongoing to try and factor few parameters out depending on which part of the detector you are in. And, for sure, the scintillator part will have to be further investigated. Coupling the PSet together means to verify, everytime we change them or add anything, that the old algorithm still works as expected, which is something I do not think is useful. I therefore propose to decouple them completely, if you agree.
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.
Sorry, but I'm missing the point.
Even if you have the parameter set description in common, the actual configurations will be different in the two cases. And if one algo needs some extra parameter not needed by the other algo, that parameter can be added to the pset description of one algo and not to the pset description of the other one.
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 shared and commonly used parameters would have to be validated twice, and, very likely, the working points will be different for the different algorithm. Also, if we introduce new parameters we will likely remove other but, if the 2 are coupled, we could not do that and will end up having stale parameters in one of the 2.
On 3/29/19 2:31 AM, perrotta wrote:
with the unphysical peak at cluster energy = 1
I expect that log10(x)==0 corresponds to x<=0
|
+upgrade |
+1
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This PR is (re)based on top of #26099. It includes several changes:
PR validation:
Tested using: