Modify HG PCL Alignment movement histograms for use in the DQM GUI #38815

mteroerd · 2022-07-21T13:54:16Z

PR description:

This PR is in preparation of style changes for the movement histograms displayed in the DQM GUI. This includes the following changes:

Fixing the order of labels in the status histogram to correctly match the variables
Adding 5 Additional bins to each movement histogram. The first additional bin is empty, the other ones include cut values on movement threshold, significance, maximum error, and maximum movement. These can be read out in the DQM GUI.

PR validation:

Mille & Pede steps were execute to produce ROOT files usable by the DQM GUI. It was confirmed that the changes to the histograms are as intended.

Will probably have to be backported to 12_4_X.

cmsbuild · 2022-07-21T14:02:14Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38815/31188

This PR adds an extra 16KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38815/31188/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38815/31188/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2022-07-21T14:15:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38815/31189

This PR adds an extra 16KB to repository

cmsbuild · 2022-07-21T14:15:38Z

A new Pull Request was created by @mteroerd (Marius Teroerde) for master.

It involves the following packages:

Alignment/MillePedeAlignmentAlgorithm (alca)

@malbouis, @yuanchao, @cmsbuild, @francescobrivio, @ChrisMisan, @tvami can you please review it and eventually sign? Thanks.
@mmusich, @adewit, @tocheng, @tlampen this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

mmusich · 2022-07-21T14:25:32Z

test parameters:

workflows = 1001.2, 1001.3

mmusich · 2022-07-21T14:26:16Z

type trk, bug-fix

mmusich · 2022-07-21T14:26:24Z

@cmsbuild, please test

cmsbuild · 2022-07-21T18:42:55Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4088c1/26372/summary.html
COMMIT: ddbc7cf
CMSSW: CMSSW_12_5_X_2022-07-21-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38815/26372/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4088c1/1001.2_RunZeroBias2017F+RunZeroBias2017F+TIER0EXPRUN2+ALCAEXPRUN2+ALCAHARVDSIPIXELCAL+ALCAHARVDSIPIXELCALLA+ALCAHARVD4+ALCAHARVDSIPIXELALIHG
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4088c1/1001.3_RunSingleMuon2022B+RunSingleMuon2022B+TIER0EXPRUN3+ALCAEXPRUN3+ALCAHARVDEXPRUN3
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4088c1/13234.0_TTbar_14TeV+2021FS+TTbar_14TeV_TuneCP5_FastSimRun3+HARVESTFastRun3
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4088c1/13434.0_TTbar_14TeV+2021FSPU+TTbar_14TeV_TuneCP5_FastSimRun3PU+HARVESTFastRun3PU

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 53
DQMHistoTests: Total histograms compared: 3721419
DQMHistoTests: Total failures: 14
DQMHistoTests: Total nulls: 54
DQMHistoTests: Total successes: 3721329
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 1.172 KiB( 52 files compared)
DQMHistoSizes: changed ( 1001.2 ): 1.172 KiB AlCaReco/SiPixelAliHG
Checked 221 log files, 47 edm output root files, 53 DQM output files
TriggerResults: no differences found

tvami · 2022-07-21T18:47:42Z

urgent

backport should go into the next 12_4_X

tvami · 2022-07-21T18:51:19Z

I see this in the SA

Dead increment /Alignment/MillePedeAlignmentAlgorithm/src/MillePedeFileReader.cc initializeIndexHelper 504 1 View Report

const_cast used on pointer to class /Alignment/MillePedeAlignmentAlgorithm/src/PedeSteererWeakModeConstraints.cc unknown 537 1 View Report

For a follow-up PR maybe @mteroerd ?

tvami · 2022-07-21T18:52:53Z

+alca

tests pass, changes in 1001.2 are expected, the rest is MsgLogger and MTD
some suggestions for a follow-up PR in Modify HG PCL Alignment movement histograms for use in the DQM GUI #38815 (comment)

cmsbuild · 2022-07-21T18:53:16Z

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

tvami · 2022-07-21T18:53:38Z

@mteroerd please prepare the 12_4_X backport, thanks!

mmusich · 2022-07-21T19:13:39Z

const_cast used on pointer to class /Alignment/MillePedeAlignmentAlgorithm/src/PedeSteererWeakModeConstraints.cc unknown 537 1 View Report

@tvami please clarify what's the expected pattern here. Thanks

perrotta · 2022-07-21T20:00:08Z

+1

Modify HG PCL Alignment movement histograms for use in the DQM GUI (Backport of #38815 )

makortel · 2022-07-27T14:03:07Z

const_cast used on pointer to class /Alignment/MillePedeAlignmentAlgorithm/src/PedeSteererWeakModeConstraints.cc unknown 537 1 View Report

@tvami please clarify what's the expected pattern here. Thanks

(@tvami asked via mattermost) I took a quick look on the MomentumDependentPedeLabeler and RunRangeDependentPedeLabeler (the only classes I found inheriting from PedeLabelerBase). Both classes use the argument Alignable * as an argument to two std::map<Alignable *, X>::find() (with different X). As far as I can tell, in both classes all uses of these two maps give only const access to the map elements (including the key). Therefore it seems to me that the two maps could be changed to std::map<const Alignable *, X> that would allow changing

cmssw/Alignment/MillePedeAlignmentAlgorithm/interface/PedeLabelerBase.h

Lines 63 to 65 in 089a546

    
           virtual unsigned int alignableLabelFromParamAndInstance(Alignable *alignable, 
        
                                                                   unsigned int param, 
        
                                                                   unsigned int instance) const = 0;

to take const Alignable *alignable avoiding the const_cast.

dmeuser · 2022-07-28T10:26:34Z

(@tvami asked via mattermost) I took a quick look on the MomentumDependentPedeLabeler and RunRangeDependentPedeLabeler (the only classes I found inheriting from PedeLabelerBase). Both classes use the argument Alignable * as an argument to two std::map<Alignable *, X>::find() (with different X). As far as I can tell, in both classes all uses of these two maps give only const access to the map elements (including the key). Therefore it seems to me that the two maps could be changed to std::map<const Alignable *, X> that would allow changing

cmssw/Alignment/MillePedeAlignmentAlgorithm/interface/PedeLabelerBase.h

Lines 63 to 65 in 089a546

virtual unsigned int alignableLabelFromParamAndInstance(Alignable *alignable,

unsigned int param,

unsigned int instance) const = 0;

to take const Alignable *alignable avoiding the const_cast.

If I'm not mistaken, one would need change all Alignable * to const Alignable * in PedeLabeler, MomentumDependentPedeLabeler, RunRangeDependentPedeLabeler. This would in the end lead to problems when using the alignableFromLabel method in

cmssw/Alignment/MillePedeAlignmentAlgorithm/src/PedeReader.cc

Line 166 in 089a546

Alignable *alignable = myLabels.alignableFromLabel(paramLabel);

But maybe I am missing something.

makortel · 2022-07-28T18:35:51Z

Thanks, so I missed all code that actually needs non-const access to the Alignable*. My next suggestion would be to define an explicit Comparator class to use in the map type definition, along

struct AlignableComparator {
    using is_transparent = void; // needs to be defined, actual type is not relevant
    bool operator()(Alignable* a, Alignable* b) const {
        return a < b;
    }
    bool operator()(Alignable* a, const Alignable* b) const {
        return a < b;
    }
    bool operator()(const Alignable* a, Alignable* b) const {
        return a < b;
    }
};

typedef std::map<Alignable *, unsigned int, AlignableComparator> AlignableToIdMap;

The important part is the AlignableComparator::is_transparent type alias, which is needed for std::map::find() to allow other argument types as long as they work together with the explicit Comparator (this feature was added for std::map in C++14, and for std::unordered_map in C++17). i have here a toy example in Compiler Explorer.

But I'd like to add that a const member function returning a pointer-to-non-const object that is held in the member data of the class is a code smell. It is hard to tell without a deep analysis on the code (possibly extending to all alignment code) how const-correct these patterns are.

dmeuser · 2022-07-29T10:07:50Z

Thanks, so I missed all code that actually needs non-const access to the Alignable*. My next suggestion would be to define an explicit Comparator class to use in the map type definition, along
struct AlignableComparator {
    using is_transparent = void; // needs to be defined, actual type is not relevant
    bool operator()(Alignable* a, Alignable* b) const {
        return a < b;
    }
    bool operator()(Alignable* a, const Alignable* b) const {
        return a < b;
    }
    bool operator()(const Alignable* a, Alignable* b) const {
        return a < b;
    }
};

typedef std::map<Alignable *, unsigned int, AlignableComparator> AlignableToIdMap;
The important part is the AlignableComparator::is_transparent type alias, which is needed for std::map::find() to allow other argument types as long as they work together with the explicit Comparator (this feature was added for std::map in C++14, and for std::unordered_map in C++17). i have here a toy example in Compiler Explorer.

But I'd like to add that a const member function returning a pointer-to-non-const object that is held in the member data of the class is a code smell. It is hard to tell without a deep analysis on the code (possibly extending to all alignment code) how const-correct these patterns are.

In #38894 this should be fixed now.

Modify HG movement histograms for DQM GUI

43704a2

cmsbuild added this to the CMSSW_12_5_X milestone Jul 21, 2022

cmsbuild added alca-pending code-checks-pending orp-pending pending-signatures tests-pending labels Jul 21, 2022

mteroerd mentioned this pull request Jul 21, 2022

Changes to the style of HG PCL Alignment plots dmwm/deployment#1171

Merged

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jul 21, 2022

Implement code format fixes

ddbc7cf

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jul 21, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 21, 2022

cmsbuild added bug-fix tests-started trk and removed tests-pending labels Jul 21, 2022

cmsbuild added tests-approved and removed tests-started labels Jul 21, 2022

cmsbuild added the urgent label Jul 21, 2022

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Jul 21, 2022

cmsbuild added orp-approved and removed orp-pending labels Jul 21, 2022

cmsbuild merged commit 34ddd51 into cms-sw:master Jul 21, 2022

mteroerd mentioned this pull request Jul 21, 2022

Modify HG PCL Alignment movement histograms for use in the DQM GUI (Backport of #38815 ) #38827

Merged

cmsbuild added a commit that referenced this pull request Jul 23, 2022

Merge pull request #38827 from mteroerd/HGPCL_GUI_BACKPORT

e63cf26

Modify HG PCL Alignment movement histograms for use in the DQM GUI (Backport of #38815 )

dmeuser mentioned this pull request Jul 29, 2022

Tracker Alignment: fix issues found by static analyzer #38894

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify HG PCL Alignment movement histograms for use in the DQM GUI #38815

Modify HG PCL Alignment movement histograms for use in the DQM GUI #38815

mteroerd commented Jul 21, 2022

cmsbuild commented Jul 21, 2022

cmsbuild commented Jul 21, 2022

cmsbuild commented Jul 21, 2022

mmusich commented Jul 21, 2022 •

edited

Loading

mmusich commented Jul 21, 2022

mmusich commented Jul 21, 2022

cmsbuild commented Jul 21, 2022

tvami commented Jul 21, 2022

tvami commented Jul 21, 2022 •

edited

Loading

tvami commented Jul 21, 2022 •

edited

Loading

cmsbuild commented Jul 21, 2022

tvami commented Jul 21, 2022

mmusich commented Jul 21, 2022

perrotta commented Jul 21, 2022

makortel commented Jul 27, 2022

dmeuser commented Jul 28, 2022

makortel commented Jul 28, 2022

dmeuser commented Jul 29, 2022

Modify HG PCL Alignment movement histograms for use in the DQM GUI #38815

Modify HG PCL Alignment movement histograms for use in the DQM GUI #38815

Conversation

mteroerd commented Jul 21, 2022

PR description:

PR validation:

cmsbuild commented Jul 21, 2022

cmsbuild commented Jul 21, 2022

cmsbuild commented Jul 21, 2022

mmusich commented Jul 21, 2022 • edited Loading

mmusich commented Jul 21, 2022

mmusich commented Jul 21, 2022

cmsbuild commented Jul 21, 2022

Comparison Summary

tvami commented Jul 21, 2022

tvami commented Jul 21, 2022 • edited Loading

tvami commented Jul 21, 2022 • edited Loading

cmsbuild commented Jul 21, 2022

tvami commented Jul 21, 2022

mmusich commented Jul 21, 2022

perrotta commented Jul 21, 2022

makortel commented Jul 27, 2022

dmeuser commented Jul 28, 2022

makortel commented Jul 28, 2022

dmeuser commented Jul 29, 2022

mmusich commented Jul 21, 2022 •

edited

Loading

tvami commented Jul 21, 2022 •

edited

Loading

tvami commented Jul 21, 2022 •

edited

Loading