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

Update of mkFit for 12_1_0_pre2 #34921

Merged
merged 6 commits into from
Aug 25, 2021
Merged

Conversation

mmasciov
Copy link
Contributor

PR description:

This PR follows #34395 and updates mkFit for physics validation in CMSSW_12_1_0_pre2 (https://its.cern.ch/jira/browse/PDMVRELVALS-130).

In detail, this PR includes:

  • update usage of SiStrip quality DB information for mkFit;
  • enable usage of SiStrip quality DB information for mkFit;
  • enable by default only InitialStepPreSplitting, InitialStep, HighPtTripletStep and DetachedQuadStep in trackingMkFit procModifier;
  • other technical optimizations.

It requires cms-sw/cmsdist#7217 and cms-data/RecoTracker-MkFit#3

Developments have been presented at Tracking POG (https://indico.cern.ch/event/1065324/#3-mkfit-status-report) and agreed with PPD (https://indico.cern.ch/event/1063451/#8-mkfit-report).

PR validation:

MTV results for tracking-only validation in TTbar RelVal (with mkFit enabled for 4 iterations as specified above):

FYI @mtosi @vmariani @mmusich
(@slava77 @makortel)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34921/24731

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmasciov (Mario Masciovecchio) for master.

It involves the following packages:

  • Configuration/ProcessModifiers (operations)
  • RecoTracker/MkFit (reconstruction)

@perrotta, @silviodonato, @cmsbuild, @slava77, @jpata, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@fabiocos, @makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @Martin-Grunewald, @ebrondol, @gpetruc, @mmusich, @mtosi, @dgulhan this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Aug 17, 2021

test parameters:

@slava77
Copy link
Contributor

slava77 commented Aug 17, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b18488/17834/summary.html
COMMIT: 30567c1
CMSSW: CMSSW_12_1_X_2021-08-17-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34921/17834/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 9 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3000352
  • DQMHistoTests: Total failures: 11
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3000318
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

Copy link
Contributor

@jpata jpata left a comment

Choose a reason for hiding this comment

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

The reco changes in the TICL tracksters do not look related to this PR (not sure about the reason at this moment). Minor comments in the code inline.

@mtosi @vmariani please let me know if you have any comments (with this targeting pre2, closing on Monday)

Comment on lines 117 to 120
if (isBad)
LogTrace("SiStripBadComponents") << "bad apv " << apv << " on " << bs.detid;
if (isBad) {
if (lastApv == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not like this? Is there something special about LogTrace?

Suggested change
if (isBad)
LogTrace("SiStripBadComponents") << "bad apv " << apv << " on " << bs.detid;
if (isBad) {
if (lastApv == -1) {
if (isBad) {
LogTrace("SiStripBadComponents") << "bad apv " << apv << " on " << bs.detid;
if (lastApv == -1) {

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the suggestion is OK, I was moving these lines a few too many times.
LogTrace does not have additional header/footer message formatting and is easier to parse than LogDebug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in commit 91d1caf

dv.push_back({phi1, phi2, q1, q2});
};

for (int apv = 0; apv < 6; ++apv) {
Copy link
Contributor

Choose a reason for hiding this comment

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

might the apv max number be a named constant?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pieterdavid @mmusich
do we have max APVs and n strips per APV formalized in common constants somewhere?
I tried to git grep, but all that I found was hardcoded 6 and 128.

Copy link
Contributor

Choose a reason for hiding this comment

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

what about modules with less than 6 APVs?

Copy link
Contributor

Choose a reason for hiding this comment

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

do we have max APVs and n strips per APV formalized in common constants somewhere?

they are (at least) in this file: https://github.com/cms-sw/cmssw/blob/master/CalibTracker/SiStripCommon/data/SiStripDetInfo.dat.
Or you can do something like:

      unsigned short nApvs = stripDet->specificTopology().nstrips() / 128;

128 is univeral for all modules, I am not sure there's a constant somewhere.

Copy link
Contributor

@pieterdavid pieterdavid Aug 18, 2021

Choose a reason for hiding this comment

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

the constants are also here (but they aren't used much in the strips code): https://github.com/cms-sw/cmssw/blob/master/DataFormats/SiStripCommon/interface/ConstantsForHardwareSystems.h#L38-L43 (6 as max number of APVs indirectly, as 2 per optical link * max 3 of these per module - it may be worth adding)

Copy link
Contributor

Choose a reason for hiding this comment

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

what about modules with less than 6 APVs?

it shouldn't matter here, if the BadApvs is filled correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in commit 91d1caf

@cmsbuild
Copy link
Contributor

Pull request #34921 was updated. @perrotta, @cmsbuild, @slava77, @jpata, @qliphy, @fabiocos, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Aug 18, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b18488/17871/summary.html
COMMIT: 91d1caf
CMSSW: CMSSW_12_1_X_2021-08-18-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34921/17871/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3000352
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3000324
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Aug 19, 2021

Thanks for the update. I have no more comments myself. I can sign after waiting until e.g. tomorrow if something comes up from the TRK groups.

@mmasciov
Copy link
Contributor Author

mmasciov commented Aug 22, 2021

Thanks for the update. I have no more comments myself. I can sign after waiting until e.g. tomorrow if something comes up from the TRK groups.

As I understand pre2 is closing tomorrow or soon after and there was no new comment since Thursday, is there anything preventing this PR (+related ones) from being merged? Thanks.

@jpata
Copy link
Contributor

jpata commented Aug 23, 2021

+reconstruction

@perrotta
Copy link
Contributor

+operations

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

@perrotta
Copy link
Contributor

@smuzaffar should we wait for you merging cms-sw/cmsdist#7229, or will you do it as soon as we merge this PR in cmsssw?

@smuzaffar
Copy link
Contributor

@perrotta , you can also merge the cmsdist PRs ( cms-sw/cmsdist#7217 and cms-sw/cmsdist#7229 ) when you merge this one. just issue merge comment on those PRs :-)

@perrotta
Copy link
Contributor

@perrotta , you can also merge the cmsdist PRs ( cms-sw/cmsdist#7217 and cms-sw/cmsdist#7229 ) when you merge this one. just issue merge comment on those PRs :-)

Thank you @smuzaffar, I will.
Since I'm kind of new in the game, I just wanted to be sure that I did not bypass your signature without your agreement

@perrotta
Copy link
Contributor

+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.

8 participants