Ignoring raw channel ids in the unit test database dump comparison

[igv4321]

PR description:

Purely technical update of a unit test, will not affect any workflows. Avoiding raw channel id comparison in the unit test of the database dump, as these ids can change.

PR validation:

Unit test was successfully run.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43220/37562

• This PR adds an extra 12KB to repository

[cmsbuild]

A new Pull Request was created by @igv4321 (Igor Volobouev) for master.

It involves the following packages:

• CondTools/Hcal (db)

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

cms-bot commands are listed here

[perrotta]

Thank you @igv4321
As soon as @bsunanda will revert the commit that comments out the unit test in #43200, these two PRs can be tested together.
One question: why do you need to ignore the raw channel ids in the comparison? IOW, why is it not possible to modify the reference file CondTools/Hcal/data/hcalpfcuts.txt to take into account the modified raw channel IDs?

[igv4321]

@bsunanda answered why raw channel ids should be ignored in #43200 (comment)

It is possible to modify CondTools/Hcal/data/hcalpfcuts.txt, but if raw channel ids are to be ignored, there is no point in doing that.

[bsunanda]

I am a bit confused now. Should I bring back the Unit test? Please let me know

…

________________________________ From: Igor Volobouev ***@***.***> Sent: 08 November 2023 13:20 To: cms-sw/cmssw ***@***.***> Cc: Sunanda Banerjee ***@***.***>; Mention ***@***.***> Subject: Re: [cms-sw/cmssw] Ignoring raw channel ids in the unit test database dump comparison (PR #43220) @bsunanda<https://github.com/bsunanda> answered why raw channel ids should be ignored in #43200 (comment)<#43200 (comment)> It is possible to modify CondTools/Hcal/data/hcalpfcuts.txt, but if raw channel ids are to be ignored, there is no point in doing that. — Reply to this email directly, view it on GitHub<#43220 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGMZOWKP6AXNQRO35ABCGDYDM2WBAVCNFSM6AAAAAA7BVGUSCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBRGI2TQNJTGM>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[perrotta]

I am a bit confused now. Should I bring back the Unit test? Please let me know

Yes @bsunanda please bring back the unit test: it will succeed once this PR gets merged

[perrotta]

@bsunanda answered why raw channel ids should be ignored in #43200 (comment)

What I did not understand from that answer is the following:

• is the comparison of the rawID values useless, in that case it is perfectly fine to remove it from the test;
• or it is still useful to be performed but it "requires changes in the code as well as [] in the data area", in which case I would apply those required changes to maintain the functionality to compare the rawID values.

In the first case, I think this PR is perfect as such. In the second case I would update it so that the comparison of the rawID values is still possible.

[igv4321]

I am not sure if the comparison of the rawID values is useful or useless, perhaps @bsunanda can comment on that. The point of this particular unit test is to ensure that the text and the database I/O work correctly for HcalPFCuts. Validation of Hcal channel mapping schemes is beyond the intended scope of this test.

[bsunanda]

Well we have to modify the DetId definition. This requires changes in the RawID value - we have to specify some bits to define the format of the RawID and that clearly changes RawId value from past to present. I am not sure if such changes will be there in the future. This is the second DetID where the internal bit definitions need to be modified. There were other DetId's which were changed - there we have to go from one class to another class. In any case, RawID is supposed to be a private member and playing with its bits should be kept provate.

…

________________________________ From: Igor Volobouev ***@***.***> Sent: 08 November 2023 15:13 To: cms-sw/cmssw ***@***.***> Cc: Sunanda Banerjee ***@***.***>; Mention ***@***.***> Subject: Re: [cms-sw/cmssw] Ignoring raw channel ids in the unit test database dump comparison (PR #43220) I am not sure if the comparison of the rawID values is useful or useless, perhaps @bsunanda<https://github.com/bsunanda> can comment on that. The point of this particular unit test is to ensure that the text and the database I/O work correctly for HcalPFCuts. Validation of Hcal channel mapping schemes is beyond the intended scope of this test. — Reply to this email directly, view it on GitHub<#43220 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGMZOQ3ELL4FMN7JBZODG3YDNH3BAVCNFSM6AAAAAA7BVGUSCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBRGQZTMMRUGM>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[perrotta]

please test with #43200

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bc0919/35691/summary.html
COMMIT: 1e4e718
CMSSW: CMSSW_14_0_X_2023-11-07-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43220/35691/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially removed 339 lines from the logs
• Reco comparison results: 140 differences found in the comparisons
• DQMHistoTests: Total files compared: 50
• DQMHistoTests: Total histograms compared: 3363010
• DQMHistoTests: Total failures: 2404
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3360584
• 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]

+db

[cmsbuild]

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

[perrotta]

@cms-sw/orp-l2 this PR is needed by #43200, but since it only removes one check from a test unit it can even be merged independently (even though having tested them together it apparently "requires externals" to get merged)

[antoniovilela]

@cms-sw/orp-l2 this PR is needed by #43200, but since it only removes one check from a test unit it can even be merged independently (even though having tested them together it apparently "requires externals" to get merged)

Thanks

[antoniovilela]

+1