Skip to content
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] CaloParticleSelector selects non-converted CP #33309

Merged
merged 3 commits into from Apr 4, 2021

Conversation

ebrondol
Copy link
Contributor

PR description:

This PR correctly identify the CaloParticle which have converted in the Tracker using the getPositionAtBoundary and getMomentumAtBoundary methods from the SimTrack associated. Before this was done looking at the number of SimClusters associated, but this method was not fully correct: in the specific case of the single photons, they always had 2 SimClusters because of pair production but not always was in the Tracker.
An exception is introduced for the electrons which most of the time undergo bremsstrahlung and therefore are always converting in the Tracker.

It would be great if this PR manage to get into 11_3_0.

PR validation:

  • Efficiency on single photons before the PR:
    image
  • Efficiency on single photons after the PR:
    image

@apsallid @lecriste @rovere @felicepantaleo

@rovere
Copy link
Contributor

rovere commented Mar 31, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33309/21839

  • This PR adds an extra 20KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ebrondol (Erica Brondolin) for master.

It involves the following packages:

Validation/HGCalValidation

@andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @jfernan2, @rvenditti can you please review it and eventually sign? Thanks.
@vandreev11, @sethzenz, @bsunanda, @rovere, @lgray, @cseez, @apsallid, @pfs, @lecriste, @hatakeyamak this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0a6364/13872/summary.html
COMMIT: 60b0b66
CMSSW: CMSSW_11_3_X_2021-03-30-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33309/13872/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS Clang-Tidy warnings: There are 1 Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0a6364/13872/llvm-analysis/cmsclangtidy.txt for details.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2640841
  • DQMHistoTests: Total failures: 4943
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2635876
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

@ebrondol apart from the clang-tidy warnings, the PR is introducing a lot of changes in HGCAL folder for phase2 samples, could you please have a look and see if there are not any unexpected changes?
Thanks

@ebrondol
Copy link
Contributor Author

ebrondol commented Mar 31, 2021

Hi @jfernan2 , unfortunately when I access the link to the GUI I have '500 Internal Server Error'. It have double checked and it seems it's not a problem from my side. Could you please let me know when the link/the GUI is fixed?

@jfernan2
Copy link
Contributor

do this link work for you?
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_11_3_X_2021-03-30-2300+0a6364/41973/dqm-histo-comparison-summary.html
Down there you have all the differences for every wf tested

@ebrondol
Copy link
Contributor Author

I can access but some of the folders are not clickable in HGCAL/HGCalValidator/ such as ticlMultiClustersFromTrackstersMerge. I can only see the ones that end with EM, HAD and Trk.

@jfernan2
Copy link
Contributor

I can see all plots, e.g.
https://tinyurl.com/ydl5or4m
Can you clarify what you mean by not clickable?
Since the folder names are very large, you should select size=large in the top left menu of the GUI

@ebrondol
Copy link
Contributor Author

ebrondol commented Apr 1, 2021

We are currently running further test. We will update the PR shortly.

@ebrondol
Copy link
Contributor Author

ebrondol commented Apr 2, 2021

@jfernan2
@apsallid and I checked the differences and we can confirmed that they are all expected.

@jfernan2
Copy link
Contributor

jfernan2 commented Apr 2, 2021

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2021

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

1 similar comment
@qliphy
Copy link
Contributor

qliphy commented Apr 4, 2021

+1

@cmsbuild cmsbuild merged commit c7abaf9 into cms-sw:master Apr 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants