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

[DD4hep] start on geometry XML payload producer #33548

Closed
wants to merge 6 commits into from

Conversation

ianna
Copy link
Contributor

@ianna ianna commented Apr 27, 2021

PR description:

@cvuosalo - IMHO, this is a very straightforward way: if xml_geometry_payload is defined, a big XML will be produced (std::cout for now).

Please, see the test:

cmsRun DetectorDescription/DDCMS/test/python/testShapes.py

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

Before submitting your pull requests, make sure you followed this checklist:

@ianna
Copy link
Contributor Author

ianna commented Apr 27, 2021

@cvuosalo - please, see 5539cc7

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33548/22329

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

@perrotta
Copy link
Contributor

-1
(please rebase in order to get rid of unwanted commits)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33548/23350

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ianna (Ianna Osborne) for master.

It involves the following packages:

Calibration/EcalCalibAlgos
DetectorDescription/DDCMS
Geometry/MTDCommonData
RecoLocalTracker/SiPixelRecHits

@perrotta, @malbouis, @civanch, @Dr15Jones, @makortel, @cvuosalo, @tlampen, @ianna, @kpedro88, @cmsbuild, @srimanob, @yuanchao, @mdhildreth, @slava77, @jpata, @pohsun, @francescobrivio can you please review it and eventually sign? Thanks.
@felicepantaleo, @argiro, @OzAmram, @thomreis, @threus, @slomeo, @vargasa, @makortel, @JanFSchulte, @simonepigazzini, @GiacomoSguazzoni, @rovere, @VinInn, @ferencek, @tocheng, @mmusich, @mtosi, @fabiocos, @rchatter, @dkotlins, @gpetruc, @tvami this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@@ -92,6 +92,8 @@ void PixelCPEFastESProducer::fillDescriptions(edm::ConfigurationDescriptions& de
// specific to PixelCPEFastESProducer
desc.add<std::string>("ComponentName", "PixelCPEFast");
desc.add<edm::ESInputTag>("MagneticFieldRecord", edm::ESInputTag());
desc.addOptional<bool>("useLAAlignmentOffsets", false)->setComment("deprecated");
Copy link
Contributor

Choose a reason for hiding this comment

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

@ianna why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think rebase didn't take into an account reverted changes :-(

@@ -105,6 +105,8 @@ void PixelCPEGenericESProducer::fillDescriptions(edm::ConfigurationDescriptions&
// specific to PixelCPEGenericESProducer
desc.add<std::string>("ComponentName", "PixelCPEGeneric");
desc.add<edm::ESInputTag>("MagneticFieldRecord", edm::ESInputTag(""));
desc.addOptional<bool>("useLAAlignmentOffsets", false)->setComment("deprecated");
Copy link
Contributor

Choose a reason for hiding this comment

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

@ianna why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think rebase didn't take into an account reverted changes:


Revert "Update mtdParameters for D49 and D60 (Bug fix)", This reverts…  … 1307d75

Revert "[backport] MTD geometry and reconstruction: synchronise with …  … faf18ac

change ECAL pedestals requireStableBeam flag for MWGR test        2a9cbcd

Revert "Revert "[backport] MTD geometry and reconstruction: synchroni…  …f2a994f

revert changes breaking online HLT menu in production release         93afbed

@@ -83,6 +83,7 @@ void PixelCPETemplateRecoESProducer::fillDescriptions(edm::ConfigurationDescript

// specific to PixelCPETemplateRecoESProducer
desc.add<std::string>("ComponentName", "PixelCPETemplateReco");
desc.addOptional<bool>("DoLorentz", true)->setComment("deprecated");
Copy link
Contributor

Choose a reason for hiding this comment

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

@ianna why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same reason as above

@fabiocos
Copy link
Contributor

@ianna why restoring old parameters in mtdParameters.xml?

@fabiocos
Copy link
Contributor

I guess it is a similar issue as in the discussion with @mmusich ... There is no reason to go back anyway

@ianna
Copy link
Contributor Author

ianna commented Jun 16, 2021

I guess it is a similar issue as in the discussion with @mmusich ... There is no reason to go back anyway

Yes, I have no idea why a rebase picked the other commits. I'll try to rebase again a bit later. If this will not work, I'll cherry-pick my commit and open a new PR.

@civanch
Copy link
Contributor

civanch commented Jun 16, 2021

@ianna , better to start a new PR even if extra work will be needed.

@slava77
Copy link
Contributor

slava77 commented Jun 17, 2021

-1

it looks like the rebase went wrong and changes in reco code are not expected

@srimanob
Copy link
Contributor

srimanob commented Jul 8, 2021

@ianna
Do we still need this, or should it be closed? Thanks.

@ianna ianna closed this Jul 9, 2021
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.

10 participants