Port COCOA to rely on DD4hep #30507
Port COCOA to rely on DD4hep #30507
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30507/16703
|
|
A new Pull Request was created by @ghugo83 for master. It involves the following packages: Alignment/CocoaApplication @cmsbuild, @pohsun, @christopheralanwest, @tocheng, @tlampen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins.
|
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
Alignment/CocoaApplication/README.md
Outdated
|
|
||
| ## Run COCOA | ||
| This does not pretend to be exhaustive. | ||
| It is what I could quickly recover in the context of moving COCOA to dd4hep (checking against regressions). |
There was a problem hiding this comment.
@ghugo83 - Deciding whether to write in the first, second or third person depends on how formal you want the document to be.
First person – I, me, we
Second person – You
Third person – He, she, they, them
First person writing, using I’ or ‘we’, means the writer has a central role in the document, therefore the writing cannot be objective. First person writing is used in reports, memos, business letters, and some types of academic writing, where the writer wants to establish his or her credentials or opinions.
Writing in the second person means you address your reader directly. It gives the document a casual, friendly tone, which helps the reader to focus on the content. It is appropriate for writing instructions, memos and how-to manuals. However, this style isn’t appropriate for other, more formal, types of technical writing.
Academic writing, scientific reports, business reports and some types of reference books usually require a more detached, measured tone. You don’t need (or want) to create a personal bond with the reader in these types of writing; certainly the reader will not want to be overly aware of your presence. Second or first person writing would most likely sound presumptuous, awkward and unprofessional. Writing in the third person creates an appropriate sense of distance or formality between the reader and the writer.
| @@ -0,0 +1,44 @@ | |||
| #ifndef CocoaAnalyser_HH | |||
There was a problem hiding this comment.
@ghugo83 - Usually, the guard includes the Subsystem and Package names and it must correspond to the class name. Something like:
#ifdef ALIGNMENT_COCOA_APPLICATION_COCOA_ANALYZER_H
| private: | ||
| OpticalAlignments oaList_; | ||
| OpticalAlignMeasurements measList_; | ||
| std::string theCocoaDaqRootFileName; |
There was a problem hiding this comment.
@ghugo83 - Please, use consistent naming: either with a trailing underscore name_ or with a prefix the - theName
| errors.size() == quality.size()) { | ||
| for (size_t i = 0; i < names.size(); ++i) { | ||
| double dimFactor = 1.; | ||
| const std::string& type = dims.at(i); |
There was a problem hiding this comment.
@ghugo83 - it's safe to use operator[] - line 296 checks that the sizes are identical.
| } else if (type == "angles" || type == "angle" || type == "nodim") { | ||
| dimFactor = 1.; | ||
| } | ||
| oaParam.value_ = values.at(i) * dimFactor; |
There was a problem hiding this comment.
@ghugo83 - same here - there is no need for bounds checking
| bool pFound = false; | ||
| //----- Look for the extra parameter in XML oaInfo that has the same name | ||
| std::string oaName = (*itoap1).name_; | ||
| for (itoap2 = extraEntXML->begin(); itoap2 != extraEntXML->end(); ++itoap2) { |
There was a problem hiding this comment.
@ghugo83 - Please, consider lambda
| OpticalAlignInfo* CocoaAnalyzer::findOpticalAlignInfoXML(const OpticalAlignInfo& oaInfo) { | ||
| OpticalAlignInfo* oaInfoXML = nullptr; | ||
|
|
||
| for (auto& myOpticalAlignInfo : oaList_.opticalAlignments_) { |
There was a problem hiding this comment.
@ghugo83 - Please, consider an std algorithm find_if?
| @@ -89,15 +89,15 @@ class ALIUtils { | |||
| static ALIdouble getMaximumDeviationDerivative() { return maximum_deviation_derivative; } | |||
| static void setMaximumDeviationDerivative(ALIdouble val) { maximum_deviation_derivative = val; } | |||
|
|
|||
| static std::vector<double> getRotationAnglesFromMatrix(CLHEP::HepRotation& rmLocal, | |||
| static std::vector<double> getRotationAnglesFromMatrix(const CLHEP::HepRotation& rmLocal, | |||
There was a problem hiding this comment.
@ghugo83 - It would be good to replace HepRotation with ROOT::Math::Rotation3D for consistency
| * WARNING: This stops at the first relevant SpecPar section encountered. | ||
| * Hence, if A GIVEN NODE HAS SEVERAL SPECPAR XML SECTIONS RE-DEFINING THE SAME PARAMETER, | ||
| * only the first XML SpecPar block will be considered. | ||
| */ |
There was a problem hiding this comment.
@ghugo83 - Hmm... if a Parameter defined multiple times a 'Vector` is created.
There was a problem hiding this comment.
Hmm within the same SpecPar block, multiple parameters values are stored in a std::vector.
But a cms::DDSpecParRefs itself is a std::vector<const DDSpecPar*>, ie gathering several SpecPar blocks.
| const std::string& parameterName, | ||
| const unsigned int parameterValueIndex) { | ||
| const std::vector<T>& allParameterValues = | ||
| getAllParameterValuesFromSpecParSections<T>(allSpecParSections, nodePath, parameterName); |
There was a problem hiding this comment.
@ghugo83 - I'm not sure if the order is guaranteed.
There was a problem hiding this comment.
Thanks @ianna for the review!
Yes the thing is that apart from the README and getAllParameterValuesFromSpecParSections, all the other comments address (very old) code which is not mine, it was already like that in 10_3_X:
https://github.com/cms-sw/cmssw/blob/CMSSW_10_3_X/Alignment/CocoaApplication/interface/CocoaAnalyzer.h
https://github.com/cms-sw/cmssw/blob/CMSSW_10_3_X/Alignment/CocoaApplication/src/CocoaAnalyzer.cc
When CocoaAnalyzer is introduced back after #25356 , it also contains the sections which were not modified in the context of the to port to DD4hep.
But ok, will address it :)
There was a problem hiding this comment.
@ianna Ok I addressed the review :)
Regarding the commits: is it possible to just do a squash-and-merge when merging the PR?
There was a problem hiding this comment.
@ghugo83 - Thanks! Here is the link to CMS FAQ http://cms-sw.github.io/faq.html#how-do-i-collapse-multiple-commits-into-one
Note, it's considered an advanced user question :-)
There was a problem hiding this comment.
@ianna There are repos with a github feature called 'Squah and merge' allowed. But dont know if that is the case for CMSSW.
Ok but usually when I do git --amend, I cannot push to the remote github branch anymore, without doing a forced push. Will it still be fine, given an open PR?
There was a problem hiding this comment.
Ok all my work is now in the second commit :)
cmsbuild
added
code-checks-pending
comparison-pending
and removed
code-checks-approved
comparison-available
tests-approved
labels
|
@cmsbuild please test |
|
The tests are being triggered in jenkins.
|
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
assign geometry Since this PR involves DD4hep, I would like to have a sign-off from geometry prior to signing off for alca. |
|
New categories assigned: geometry @Dr15Jones,@cvuosalo,@mdhildreth,@makortel,@ianna,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks |
|
+1 |
|
+1 |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
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. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
Moved COCOA DD dependencies to DD4hep.
As discussed in #25356.
General description of COCOA:
https://cds.cern.ch/record/1047121/files/p193.pdf
COCOA is now basically standalone alignment software.
It is designed to simulate and reconstruct optical alignment systems for CMS. It was also used (in the past I presume) to analyze the CMS optical alignment data, and pass the alignment correction to the CMS reconstruction.
PR description:
Following was done for port to DD4hep (this can also be seen as a HOWTO):
PR validation:
Had to 'reconstruct' input files to be able to run COCOA.
Notably created an input XML file + an OpticalAlignmentsRcd stored in local .db.
Created small guide: Alignment/CocoaApplication/README.md, so that one can now easily run COCOA and reproduce results.
No regression due to port to DD4hep:
Bugs in COCOA:
While checking for regressions, I spotted & fixed the following issues in COCOA (independent from port to DD4hep). Visible in the code before PR:
colX, colY and colZ are obviously not columns, but rows.
This results in the rotation matrix rotclhep to be ill-formed (transpose, hence inverse, of what it should be). Hence, the local rotations in ideal geometry were inverse of what they should be, and could not be properly checked against those of the geometry from DB.
This is used to write results to DB. Error in the conversion (!!!), should go from m (in COCOA) to cm (in DB). Hence data stored in DB was wrong...
std::string type is initialized with value from previous iteration. Hence, within the loop, there is a telescopage of its values. This leads to dimFactor being not properly calculated, hence all 'extra alignment parameters' values were wrong.
Disclaimer:
The COCOA packages were very bad style, I re-wrote and commented only part of them, in the context of the port to DD4hep. Notably, src/CocoaAnalyzer.cc was removed then restored, hence I am partially attributed parts of old code there which is not mine.
I fully debugged COCOA behavior in the fields of ideal geometry building + measurements corrections + results write to DB. Though, there are other important aspects in the alignment which I did not debug (notably the least-square parameters fitting).