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.

Sign up for GitHub

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

Jump to bottom

Phase2 Trigger Primitive generation for Ecal Barrel #42633

Merged
merged 3 commits into from
Dec 19, 2023
Merged

Phase2 Trigger Primitive generation for Ecal Barrel #42633

merged 3 commits into from
Dec 19, 2023

Conversation

nancymarinelli
Copy link
Contributor

@nancymarinelli nancymarinelli commented Aug 22, 2023 •
edited
Loading

PR description:

This PR addresses (although probably not entirely) the emulation of the TP generation with the EB upgraded off detector electronics (BCP). We implemented the algorithms (as we know them now) which will be running on the BCP.
In doing this we decided not to reinvent the wheel and try to follow (as long as it was possible) what done for PhaseI (structure wise)
Two packages, SimCalorimetry/EcalEBTrigPrimProducers and SimCalorimetry/EcalEBTrigPrimAlgos were already existing since a long time with algorithms based on hacking the Phase I algos and using the Phase I digis. We keep those files
in the packages until the development of the new stuff is completed.
We have added new files (.cc and .h with a clear Phase2 name in them.
The packages SimCalorimetry/EcalEBTrigPrimProducers contain the producer and the ES producers while
SimCalorimetry/EcalEBTrigPrimAlgos contains all the algorithmic modules. The main input to all this are the Phase 2 digis.
At this point in time we are not yet interfaced with the brand new digis made out of the new signal shape.

In DataFormats/EcalDigis the new EBTrigPrim formats were already present since long time but we had to apply some
updates.
New files were added to CondFormats/DataRecors and CondFormats/EcalObjects to provide the necessary environment
for the new algorithms.
In SimCalorimetry/Configuration/python the ecalDigiSequence_cff.py has been update to run the Phase2 TP sequence
and a new Era modifier has been added in Configuration/Eras , called phase2_ecalTP_devel (so to keep the TP generation independent from the phase2_ecal_devel which mainly drives the digi production)
We consider this PR only a starting point: a) many details strictly related to the final electronics are missing, because they are not yet fully known, b) all what is related to databases is missing and I surely am forgetting many other items.
It's also very possible that this PR will be updated in some places in the very near future

This PR needs to be merged -together- with
cms-data/SimCalorimetry-EcalEBTrigPrimProducers#1

PR validation:

The PR has been tested with a private .cfg (also committed in SimCalorimetry/EcalEBTrigPrimProducers/test).
I also tried cmsDriver with the workflow 24234.61. For step2 I did not manage to run it in automatic. If I add to the eras options the new phase2_ecalTP_devel, cmsRiver does not recognize it. But if I run the cmsDriver with --no_exec and I add
by hand the new era in the step2.cfg all works fine. There are details which I do not know about including it automated in the cmsDriver.
Finally, the code has been committed after running the scram b code-format

@cmsbuild cmsbuild added this to the CMSSW_13_3_X milestone Aug 22, 2023
@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42633/36688

  • This PR adds an extra 396KB to repository

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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42633/36690

  • This PR adds an extra 460KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 22, 2023 •
edited
Loading

A new Pull Request was created by @nancymarinelli for master.

It involves the following packages:

  • CalibCalorimetry/EBPhase2TPGTools (****)
  • CondFormats/DataRecord (db, alca)
  • CondFormats/EcalObjects (db, alca)
  • Configuration/Eras (operations)
  • DataFormats/EcalDigi (simulation)
  • SimCalorimetry/Configuration (simulation)
  • SimCalorimetry/EcalEBTrigPrimAlgos (upgrade, l1)
  • SimCalorimetry/EcalEBTrigPrimProducers (upgrade, l1)

The following packages do not have a category, yet:

CalibCalorimetry/EBPhase2TPGTools
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@perrotta, @rappoccio, @epalencia, @consuegs, @mdhildreth, @tvami, @cmsbuild, @AdrianoDee, @srimanob, @saumyaphor4252, @aloeliger, @civanch, @antoniovilela, @francescobrivio, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @rchatter, @AnnikaStein, @rovere, @argiro, @Martin-Grunewald, @missirol, @tocheng, @thomreis, @wang0jin, @mmusich, @sameasy, @fabiocos, @seemasharmafnal this is something you requested to watch as well.
@perrotta, @dpiparo, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@aloeliger
Copy link
Contributor

@nancymarinelli has this been brought up to the L1T Phase 2 DPG?

@aloeliger
Copy link
Contributor

@nancymarinelli Are there any trigger performance changes expected from this? Are there any old vs new plots we should be aware of?

@nancymarinelli
Copy link
Contributor Author

nancymarinelli commented Aug 25, 2023 via email

@nancymarinelli
Copy link
Contributor Author

nancymarinelli commented Aug 25, 2023 via email

@aloeliger
Copy link
Contributor

@nancymarinelli Then would you like this tested, but placed on hold? Or maybe this PR should be marked as a draft if it is not tested/ready for integration?

aloeliger
aloeliger reviewed Aug 28, 2023
View reviewed changes
Copy link
Contributor

@aloeliger aloeliger left a comment

Choose a reason for hiding this comment

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

If some changes are going to made to this, then there are a few things that could be tidied up. I just looked at TrigPrimProducers.

...rimProducers/data/AmpTimeOnPeakXtalWeightsCMSSWPulse_8samples_peakOnSix_WithAndyFixes.txt.gz Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This, and 3 other /data/ files will likely need to go into the externals and be removed from the git history (likely by squashing the commits).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello Andrew, I have a question on this one. Ideally the content of the files should go in the DB. But that will come much later. Can these files stay here for the time being ? If absolutely not, I would need instructions to make them external and how to access them. I am not familiar with all that. Thanks

SimCalorimetry/EcalEBTrigPrimProducers/plugins/EcalEBTrigPrimPhase2ESProducer.cc Outdated
}

EcalEBTrigPrimPhase2ESProducer::~EcalEBTrigPrimPhase2ESProducer() {
std::cout << " EcalEBTrigPrimPhase2ESProducer DTOR " << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be changed to logDebug or logInfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

SimCalorimetry/EcalEBTrigPrimProducers/plugins/EcalEBTrigPrimPhase2ESProducer.cc Outdated
Comment on lines 314 to 315
std::cout << " EcalEBTrigPrimPhase2ESProducer::producePhysicsConst Needs updating if we want to keep it "
<< std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be logDebug or logInfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

SimCalorimetry/EcalEBTrigPrimProducers/plugins/EcalEBTrigPrimPhase2ESProducer.cc Outdated
finalFileName = fileInPath.fullPath();
} else {
finalFileName = configFilename_;
std::cout << "Couldnt find database file via fileinpath trying with pathname directly!!" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be logDebug, logInfo, or logError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

SimCalorimetry/EcalEBTrigPrimProducers/plugins/EcalEBTrigPrimPhase2ESProducer.cc
gis >> std::dec >> id;

if (flagPrint_) {
std::cout << dataCard << " " << std::dec << id << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be logDebug or logInfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flagPrint_ is set to false by default

SimCalorimetry/EcalEBTrigPrimProducers/plugins/EcalEBTrigPrimPhase2ESProducer.cc
gis >> std::dec >> id;

if (flagPrint_) {
std::cout << dataCard << " " << std::dec << id << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about log versus cout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flagPrint_ is set to false by default

SimCalorimetry/EcalEBTrigPrimProducers/plugins/EcalEBTrigPrimPhase2ESProducer.cc
Comment on lines +472 to +389
std::cout << st6 << std::endl;
std::cout << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

This, and all remaining cout instances should probably be changed to log statements instead

Copy link
Contributor Author

@nancymarinelli nancymarinelli Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@perrotta @aloeliger These couts are included in a if(flagPrint) which is set to false by default. And in other pieces of code tehre are many couts inclosed in a if (debug) which is set to false by default. We cannot even have the liberty to switch on defug lines in our private areas ?? Where possible I have change cout with logInfo or logWarning. All the others can, as I said, switched on only for our private purposes (until V0 will become V final)

SimCalorimetry/EcalEBTrigPrimProducers/plugins/EcalEBTrigPrimPhase2ESProducer.cc Outdated
Comment on lines 125 to 199
std::unique_ptr<EcalTPGSlidingWindow> EcalEBTrigPrimPhase2ESProducer::produceSlidingWindow(
const EcalTPGSlidingWindowRcd &iRecord) {
auto prod = std::make_unique<EcalTPGSlidingWindow>();
parseTextFile();
for (int subdet = 0; subdet < 2; subdet++) {
std::map<uint32_t, std::vector<uint32_t>>::const_iterator it;
for (it = mapStrip_[subdet].begin(); it != mapStrip_[subdet].end(); it++) {
prod->setValue(it->first, (it->second)[0]);
}
}
return prod;
}

std::unique_ptr<EcalTPGFineGrainEBIdMap> EcalEBTrigPrimPhase2ESProducer::produceFineGrainEB(
const EcalTPGFineGrainEBIdMapRcd &iRecord) {
auto prod = std::make_unique<EcalTPGFineGrainEBIdMap>();
parseTextFile();
EcalTPGFineGrainConstEB fg;
std::map<uint32_t, std::vector<uint32_t>>::const_iterator it;
for (it = mapFg_.begin(); it != mapFg_.end(); it++) {
fg.setValues((it->second)[0], (it->second)[1], (it->second)[2], (it->second)[3], (it->second)[4]);
prod->setValue(it->first, fg);
}
return prod;
}

std::unique_ptr<EcalTPGFineGrainStripEE> EcalEBTrigPrimPhase2ESProducer::produceFineGrainEEstrip(
const EcalTPGFineGrainStripEERcd &iRecord) {
auto prod = std::make_unique<EcalTPGFineGrainStripEE>();
parseTextFile();
// EE Strips
std::map<uint32_t, std::vector<uint32_t>>::const_iterator it;
for (it = mapStrip_[1].begin(); it != mapStrip_[1].end(); it++) {
EcalTPGFineGrainStripEE::Item item;
item.threshold = (it->second)[2];
item.lut = (it->second)[3];
prod->setValue(it->first, item);
}
// EB Strips
for (it = mapStrip_[0].begin(); it != mapStrip_[0].end(); it++) {
EcalTPGFineGrainStripEE::Item item;
item.threshold = (it->second)[2];
item.lut = (it->second)[3];
prod->setValue(it->first, item);
}
return prod;
}

std::unique_ptr<EcalTPGFineGrainTowerEE> EcalEBTrigPrimPhase2ESProducer::produceFineGrainEEtower(
const EcalTPGFineGrainTowerEERcd &iRecord) {
auto prod = std::make_unique<EcalTPGFineGrainTowerEE>();
parseTextFile();
std::map<uint32_t, std::vector<uint32_t>>::const_iterator it;
for (it = mapTower_[1].begin(); it != mapTower_[1].end(); it++) {
prod->setValue(it->first, (it->second)[1]);
}
return prod;
}

std::unique_ptr<EcalTPGLutIdMap> EcalEBTrigPrimPhase2ESProducer::produceLUT(const EcalTPGLutIdMapRcd &iRecord) {
auto prod = std::make_unique<EcalTPGLutIdMap>();
parseTextFile();
EcalTPGLut lut;
std::map<uint32_t, std::vector<uint32_t>>::const_iterator it;
for (it = mapLut_.begin(); it != mapLut_.end(); it++) {
unsigned int lutArray[1024];
for (int i = 0; i < 1024; i++)
lutArray[i] = (it->second)[i];
lut.setLut(lutArray);
prod->setValue(it->first, lut);
}
return prod;
}

*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Large chunks of commented code like this should be removed unless they serve a purpose to illustrate something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@aloeliger
Copy link
Contributor

please test

  • After conversation with @nancymarinelli I believe that L1T is fine with this. Just running the tests to see if there's anything amiss there.

@srimanob
Copy link
Contributor

+Upgrade

@srimanob
Copy link
Contributor

@cms-sw/simulation-l2 @cms-sw/l1-l2
Could you please consider and (re-)sign? Thanks.

@perrotta
Copy link
Contributor

NOTE for the release managers: this has to be merged together with cms-data/SimCalorimetry-EcalEBTrigPrimProducers#1

(@nancymarinelli , if you write it in the PR description, for which I have no rights to do so, the accompanying cms-data PR can be spotted more easily)

@epalencia
Copy link
Contributor

+l1

@nancymarinelli
Copy link
Contributor Author

@mdhildreth, @fabiocos, @perrotta, @antoniovilela, @civanch, @saumyaphor4252, @consuegs, @srimanob, @francescobrivio, @aloeliger, @epalencia, @davidlange6, @rappoccio, @subirsarkar
This PR needs to be merged together with cms-data/SimCalorimetry-EcalEBTrigPrimProducers#1

@civanch
Copy link
Contributor

civanch commented Dec 14, 2023

+1

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-234212/36555/summary.html
COMMIT: e41887c
CMSSW: CMSSW_14_0_X_2023-12-18-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42633/36555/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@antoniovilela
Copy link
Contributor

+1

@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 be automatically merged.

@cmsbuild cmsbuild merged commit deaac86 into cms-sw:master Dec 19, 2023
12 checks passed
@cmsbuild cmsbuild mentioned this pull request Dec 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makortel makortel makortel left review comments

@Dr15Jones Dr15Jones Dr15Jones left review comments

@perrotta perrotta perrotta left review comments

@thomreis thomreis thomreis left review comments

@srimanob srimanob srimanob left review comments

@francescobrivio francescobrivio francescobrivio left review comments

@jhakala jhakala jhakala left review comments

@aloeliger aloeliger aloeliger left review comments

@LDLutton LDLutton LDLutton left review comments

Assignees
No one assigned
Labels
alca-approved code-checks-approved db-approved ecal fully-signed l1-approved operations-approved orp-approved requires-external simulation-approved tests-approved upgrade-approved
Projects
None yet
Milestone
CMSSW_14_0_X
Development

Successfully merging this pull request may close these issues.