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

ETL Validation code update and fixes #32476

Merged
merged 4 commits into from Dec 17, 2020

Conversation

gsorrentino18
Copy link
Contributor

PR description:

The main changes this PR introduces in the code are the following (@fabiocos, @parbol):

  1. The MeVtoMIP conversion value to be used for the new 2-discs ETL geometry validation has been fixed to the correct value of 0.015.

  2. The filling of the occupancy plots has been modified, introducing a negative weight to discriminate between the two sides of each ETL disc.

  3. The MTDGlobalReco plugins have been corrected, merging the two separate histograms used in the evaluation of the efficiencies, one for each of the two ETL discs, into a single one. This has been done since the efficiency is evaluated as:
    Tracks with an associated deposit in negative (positive) side of ETL MTD/ All tracks with -3.2 < η < -1.5 (1.5 < η < 3.2),
    where the numerator does not discriminate between tracks with an associated hit in the first or second ETL disc.

PR validation:

The new code has been tested in the release CMSSW_11_2_0_pre10

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32476/20381

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @gsorrentino18 (Giulia Sorrentino) for master.

It involves the following packages:

RecoLocalFastTime/FTLRecProducers
SimFastTiming/FastTimingCommon
Validation/MtdValidation

@perrotta, @andrius-k, @kmaeshima, @civanch, @ErnestaP, @mdhildreth, @cmsbuild, @jfernan2, @fioriNTU, @slava77, @jpata, @kpedro88 can you please review it and eventually sign? Thanks.
@fabiocos 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

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16edc8/11615/summary.html
CMSSW: CMSSW_11_3_X_2020-12-14-1100/slc7_amd64_gcc900

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 7 differences found in the comparisons
  • DQMHistoTests: Total files compared: 36
  • DQMHistoTests: Total histograms compared: 2747212
  • DQMHistoTests: Total failures: 18
  • DQMHistoTests: Total nulls: 19
  • DQMHistoTests: Total successes: 2747153
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 474.895 KiB( 35 files compared)
  • DQMHistoSizes: changed ( 23234.0,... ): 162.715 KiB MTD/ETL
  • DQMHistoSizes: changed ( 23234.0,... ): -4.418 KiB MTD/GlobalReco
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 153 log files, 37 edm output root files, 36 DQM output files

@jfernan2
Copy link
Contributor

+1

@@ -214,13 +215,19 @@ void EtlSimHitsValidation::analyze(const edm::Event& iEvent, const edm::EventSet
convertMmToCm((hit.second).x), convertMmToCm((hit.second).y), convertMmToCm((hit.second).z));
const auto& global_point = thedet->toGlobal(local_point);

if (topo2Dis) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could combine these conditions:

if (topo2Dis and detId.discSide() == 1)
  weight = -weight;

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16edc8/11665/summary.html
CMSSW: CMSSW_11_3_X_2020-12-14-2300/slc7_amd64_gcc900

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 36
  • DQMHistoTests: Total histograms compared: 2747212
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 18
  • DQMHistoTests: Total successes: 2747165
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 474.891 KiB( 35 files compared)
  • DQMHistoSizes: changed ( 23234.0,... ): 162.715 KiB MTD/ETL
  • DQMHistoSizes: changed ( 23234.0,... ): -4.418 KiB MTD/GlobalReco
  • Checked 153 log files, 37 edm output root files, 36 DQM output files

@jfernan2
Copy link
Contributor

+1

@jpata
Copy link
Contributor

jpata commented Dec 15, 2020

+reconstruction

  • reco changes are minimal and limited to the updating of the calibrationConstant value
  • the changes are under the phase2_etlV4 modifier, we see no reco changes as expected (there seems to be no matrix workflow for this modifier)

@fabiocos
Copy link
Contributor

@jpata this PR is supposed to affect scenarios D72 and D73, which are not in the short matrix so far (they are all available in the upgrade section of runTheMatrix.py)

@civanch
Copy link
Contributor

civanch commented Dec 15, 2020

+1

@jpata
Copy link
Contributor

jpata commented Dec 16, 2020

this PR is supposed to affect scenarios D72 and D73, which are not in the short matrix so far (they are all available in the upgrade section of runTheMatrix.py)

@fabiocos
OK, I see it now, phase2_etlV4 -> Phase2C11_etlV4 -> 2026D72, 2026D73, my original grep for the modifier itself did not bring it up, thanks for pointing it out. Nevertheless, it does not affect the reco signature (minimal changes protected by ETL-specific modifiers).

@fabiocos
Copy link
Contributor

@jpata in case one can run here for instance wf 33434.0, but we will not see a comparison, where the effect on digitization should be apparent. A presentation about this was given at last MTD DPG by @gsorrentino18 https://indico.cern.ch/event/984445/contributions/4146432/attachments/2161556/3647170/MTD-DPG%2011_12_2020.pdf

@kpedro88
Copy link
Contributor

+upgrade

@cmsbuild
Copy link
Contributor

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)

@qliphy
Copy link
Contributor

qliphy commented Dec 17, 2020

+1

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

9 participants