introduce GEMChMap for GE21 chambers

[yeckang]

PR description:

• The current GEM electronics mapping can not handle the vfat that reads the signal from more than 1 unit detector module.
• In the GE21, a single vfat reads the signal from 2 different unit detector modules.
• The new electronics mapping format will be introduced with this PR.
• No changes are expected with this PR.

PR validation:

• The code format has applied with scram build code-format and scram build code-checks.
• The branch is compileable.

@jshlee @watson-ij

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37777/29677

• This PR adds an extra 44KB to repository

[cmsbuild]

A new Pull Request was created by @yeckang (Yechan Kang) for master.

It involves the following packages:

• CondCore/GEMPlugins (db)
• CondFormats/DataRecord (db, alca)
• CondFormats/GEMObjects (db)

@malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@jshlee, @mmusich, @seemasharmafnal, @watson-ij, @tocheng 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

[tvami]

code-checks

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37777/29684

• This PR adds an extra 44KB to repository

[tvami]

Hi @yeckang ,
I have some general questions:

1. is this needed for the data-taking in June/July?
2. Do you need this in 12_4_X or it's fine if it goes to the next release?

And then some a technical comment:

• as this PR already proves, it's better to write the record classes in the most general way as possible, so you dont need to modify them. I see that the public members have variables for GE0, GE11, GE21 ... what if the rest needs changes in the future?
• One "trick" to do is instead of specific variables for GEx, you'd have a vector as the member variable, whose elements are for GEx. The vector then can later be extended if needed. Please consider implementing that

[tvami]

@smuzaffar in here

code checks refers to the PR 37775, is there a way to clear that up?

[smuzaffar]

@tvami , no there is no easy way to clean up this. These statuses belong to git commits so if two or more PRs use the same branch/commit then these statuses will be visible in all those PRs. Only way to avoid it is to create separate branches for each PR

[tvami]

ok thanks @smuzaffar !

[tvami]

@yeckang @jshlee @watson-ij
please comment on #37777 (comment)
especially the

Do you need this in 12_4_X or it's fine if it goes to the next release?

part, given that 12_4_X will close very soon...

[yeckang]

Hi @tvami,
To unpack the data from GE21, this PR and the following PR(updating the packer/unpacker) are needed.
And the updated packer/unpacker requires the GEMChMap instead of GEMeMap.
So we want to release this update and upload the new mapping to the database before we make the new PR.
For that reason, we want to release It as soon as possible.

[tvami]

Hi @yeckang thanks, please find the rest of my comments copy pasted here

1. is this needed for the data-taking in June/July?

And then some a technical comment:

• as this PR already proves, it's better to write the record classes in the most general way as possible, so you dont need to modify them. I see that the public members have variables for GE0, GE11, GE21 ... what if the rest needs changes in the future?
• One "trick" to do is instead of specific variables for GEx, you'd have a vector as the member variable, whose elements are for GEx. The vector then can later be extended if needed. Please implementing something along these lines

[yeckang]

@tvami
For now, we have only 1 layer of GE21 chamber in P5 for testing. And we already taking the data from that chamber, even if we can not see any unpacked result from the cetrally processed data.
But I'm not sure if people want to have DQM for this chamber.

@jshlee Could you make it clear about it?

[jshlee]

The GEM operations did request for GE21 slice test chamber to be included in the onlineDQM, which means we need this PR.

To clean up the code-checks issue, do we close this PR, make a new branch and open a new PR?

[tvami]

To clean up the code-checks issue, do we close this PR, make a new branch and open a new PR?

No-no-no please dont open a third PR about this....

But please read my "technical comment" I wrote twice already above

[jshlee]

Hi @yeckang thanks, please find the rest of my comments copy pasted here

1. is this needed for the data-taking in June/July?

Yes, this is very much needed.

And then some a technical comment:

• as this PR already proves, it's better to write the record classes in the most general way as possible, so you dont need to modify them. I see that the public members have variables for GE0, GE11, GE21 ... what if the rest needs changes in the future?

Since the dataformat is pretty much finalised, we don't forsee any changes to these member variables.

• One "trick" to do is instead of specific variables for GEx, you'd have a vector as the member variable, whose elements are for GEx. The vector then can later be extended if needed. Please implementing something along these lines

I don't quite see the benefits of this, since it would also require changes to c++ code.
Ideally, we should be able to remove all these specific variables for GEx.

[tvami]

Thanks for the clear answers @jshlee !

Since the dataformat is pretty much finalised, we don't forsee any changes to these member variables.

What about Phase-2 changes? You'll do a next 3rd record definitions?

I don't quite see the benefits of this, since it would also require changes to c++ code.

The advantage is that you dont need to change the record definition, which is not supported anyways

Ideally, we should be able to remove all these specific variables for GEx.

Could you please clarify what this means?

[jshlee]

These changes are for phase 2.

I think I'm a bit mixed up on which definition you are referring to.
Can you point to the code to clear it up?

[tvami]

Let's trigger tests while we discuss the changes

[tvami]

@cmsbuild , please test

[tvami]

type gem

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1d2439/24450/summary.html
COMMIT: 845495a
CMSSW: CMSSW_12_4_X_2022-05-04-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37777/24450/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
39434.501 step 3
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

• No significant changes to the logs found
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 49
• DQMHistoTests: Total histograms compared: 3700548
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3700524
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
• Checked 205 log files, 45 edm output root files, 49 DQM output files
• TriggerResults: no differences found

[tvami]

These changes are for phase 2.

I meant the changes for the HL-LHC

But ok, we can deal with that in the coming years, given that this does seem to be urgent

[tvami]

+1

• PR in line with the PR description
• possible extensions for the further future are delayed due to the urgency of this update
• tests pass

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

[tvami]

urgent

[tvami]

@yeckang @jshlee please submit the backport PR to 12_3_X (with the PR title containing the string "[12_3_X]") after this PR is merged

[perrotta]

+1