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

SiPixel Payload Inspector: introduce utility to display probability-weighted SiPixelFEDChannelContainer contents #43020

Merged

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Oct 14, 2023

PR description:

The main goal of this PR is to add the class SiPixelFEDChannelContainerMapWeigthed to SiPixelFEDChannelContainer Payload Inspector plugin (this is done in commit b03913c).
This class displays an aggregate map of the masked components for all scenarios included in the examined payload, weighted on the probability per PU unit from a SiPixelQualityProbabilities payload (assuming a flat PU profile in the range encoded in SiPixelQualityProbabilities).
The SiPixelQualityProbabilities tag comes from user input and is retrieved via a local cond::persistency::Session opened in the production database.
This is useful, because without combining the information from both SiPixelStatusScenariosRcd and SiPixelStatusScenarioProbabilityRcd it is not possible a priori to determine the fraction of events in which a given ROC will be masked when the bad FED channel simulation is used (as it will be in the 2022 and 2023 physics MC).
Additionally I add a standalone testing script (in commit db7d86e) and add this class to the pre-existing unit test script of this package (in commit f06968c).
A possible extension to supply provision for a user-input list of PU weights (to make it even more adherent to actual full simulation process) is envisaged, but let for a follow-up PR.

PR validation:

This PR passes unit tests of the involved package: scram b runtests
Additionally I tested the code with this command:

getPayloadData.py \
    --plugin pluginSiPixelFEDChannelContainer_PayloadInspector \
    --plot plot_SiPixelBPixFEDChannelContainerWeightedMap \
    --tag SiPixelStatusScenarios_StuckTBMandOther_2023_v2_mc \
    --input_params '{"SiPixelQualityProbabilitiesTag": "SiPixelQualityProbabilities_2023_v2_mc"}' \
    --time_type Run \
    --iovs '{"start_iov": "1", "end_iov": "1"}' \
    --db Prod \
    --test ;

and obtained the following plot:

image

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

N/A

Cc: @tsusa

…er_PayloadInspector

- displays aggregate map of the masked components for all scenarios,
  weighted on the probability per PU unit from SiPixelQualityProbabilities
  assuming a flat PU profile in the range encoded in  SiPixelQualityProbabilities
  The SiPixelQualityProbabilities tag comes from user input.
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43020/37192

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

  • CondCore/SiPixelPlugins (db)

@francescobrivio, @saumyaphor4252, @consuegs, @cmsbuild, @perrotta can you please review it and eventually sign? Thanks.
@mmusich, @mroguljic, @yuanchao, @rsreds, @VinInn, @tvami, @ferencek, @PonIlya, @dkotlins this is something you requested to watch as well.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor Author

mmusich commented Oct 14, 2023

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8bc635/35193/summary.html
COMMIT: db7d86e
CMSSW: CMSSW_13_3_X_2023-10-14-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43020/35193/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 1 lines from the logs
  • Reco comparison results: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3356920
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3356895
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 214 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

In the log output of the newly inserted test "testPixelPayloadInspector" there are quite several LogError messages as this:

%MSG-e PixelRegionContainers: 
303042564 :=> 1000 is not a recongnized PixelId enumerator! 
PixelBarrel Layer 0 Ladder 1 Module for phase0 1 Module for phase2 1 module (303042564)
%MSG

They come from PixelRegionContainers. Do yo have any hint about them?

@mmusich
Copy link
Contributor Author

mmusich commented Oct 16, 2023

They come from PixelRegionContainers. Do yo have any hint about them?

Not from this PR

@mmusich
Copy link
Contributor Author

mmusich commented Oct 17, 2023

Ping?

@perrotta
Copy link
Contributor

They come from PixelRegionContainers. Do yo have any hint about them?

Not from this PR

I understand that this quite likely does not affect the relevant functionalities introduced by tis PR. However, it is a matter of fact that the new unit test gives some weird output, and layer 0 of the barrel pixel is searched for. Is it a problem of the input data used? Are there used some wrong conditions?
I think that, if possible, it should be fixed. If, on the contrary, you think that we should not care about it because the purposes of the unit test are fulfilled nonetheless, please add a line in the PR description about it, so that if anyone will wonder in the future why those errors are there, a reference can still be found about it.
Thank you!

@mmusich
Copy link
Contributor Author

mmusich commented Oct 17, 2023

I understand that this quite likely does not affect the relevant functionalities introduced by tis PR. However, it is a matter of fact that the new unit test gives some weird output, and layer 0 of the barrel pixel is searched for. Is it a problem of the input data used? Are there used some wrong conditions?
I think that, if possible, it should be fixed. If, on the contrary, you think that we should not care about it because the purposes of the unit test are fulfilled nonetheless, please add a line in the PR description about it, so that if anyone will wonder in the future why those errors are there, a reference can still be found about it.
Thank you!

this is completely irrelevant for this PR. If that bothers you open an issue.

@mmusich
Copy link
Contributor Author

mmusich commented Oct 17, 2023

However, it is a matter of fact that the new unit test gives some weird output,

This is not even wrong, see current IB logs.

  • the test is not new;
  • the "weird" output is already present in IBs

@perrotta
Copy link
Contributor

+1

  • Being the error message from the PixelRegionContainers in the unit tests irrelevant for this PR, no reason to not to sign it; Tracker TPG will possibly take care of it, if they consider it of any relevance to debug that code

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

@mmusich
Copy link
Contributor Author

mmusich commented Oct 17, 2023

Tracker TPG will possibly take care of it, if they consider it of any relevance to debug that code

for the record the test that triggers the verbose log is this:

histoPhase2.process(connectionString, PI::mk_input(f_tagPhase2, 1, 1, l_tagPhase2, 1, 1));

I would guess that somehow here

auto l_tTopo =
StandaloneTrackerTopology::fromTrackerParametersXMLFile(edm::FileInPath(path_toTopologyXML).fullPath());

the phase-1 topology is loaded as opposed to the phase-2. Not clear why this happens at this point, but I won't be able to debug further with limited connection I have at the moment.

@antoniovilela
Copy link
Contributor

+1

  • Error messages not related to this PR, to be followed up.

@cmsbuild cmsbuild merged commit b3c51d5 into cms-sw:master Oct 17, 2023
@mmusich mmusich deleted the dev_SiPixelFEDChannelContainerMapWeigthed branch October 17, 2023 13:25
@mmusich
Copy link
Contributor Author

mmusich commented Oct 17, 2023

Error messages not related to this PR, to be followed up.

so... it took me a while to figure out what's going on, but the issue lies in this commit:

5af9d66#diff-ce714e8f8d1e17f5c572dea87e1c0370ce098a7cad82c4326d4611db2d805b90L8

this screwed up the meaning of the bit masks in StandaloneTrackerTopology.
Will fix and also create unit tests, such that when the TrackerTopology from DB is changed we don't miss to update the class constructed from XML.

@mmusich
Copy link
Contributor Author

mmusich commented Oct 19, 2023

Will fix and also create unit tests, such that when the TrackerTopology from DB is changed we don't miss to update the class constructed from XML.

done at #43059

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

4 participants