GeoModelXML does not provide correct AlignableTransforms to GmxInterface

While debugging (mis-)alignment of the ITk with @xilin, I noticed that the pointers for AlignableTransforms we receive here:

void GmxInterface::addAlignable(int level, map<std::string, int> &index, GeoVFullPhysVol *fpv, 
                                GeoAlignableTransform * transform) {
   
    std::stringstream idString;
    // debug message
    msglog << MSG::DEBUG 
        << "GmxInterface::addAlignable called for a physvol. Logvol name " 
        << fpv->getLogVol()->getName() << ". Level = " << level << ". Indices:   ";
    for (map<string, int>::iterator i = index.begin(); i != index.end(); ++i) {
        msglog << i->second << "   ";
        if(i!=index.begin()) idString <<"_";
        idString << i->first <<"_"<<i->second;
    }
    msglog << endmsg;
    //--------------
    if(!fpv) msglog << MSG::WARNING << "Could not find a FullPhysVol when adding alignable"<<endmsg;
    if(!transform) msglog << MSG::WARNING << "Could not find an alignable transform when adding alignable"<<endmsg;
    //publish
    if (m_publisher) m_publisher->publishNode<GeoAlignableTransform *,std::string>(transform,idString.str());
}

do not seem to be correct. They seem to be in many case the same AlignableTransform pointer for many GeoFullPhysVol pointers. As a consequence, when trying to mis-align elements of the ITk in Athena, we overwrite the same AlignableTransform multiple times with many different deltas, rather than setting the deltas for the "correct" AlignableTransform for the GeoFullPhysVol we are considering.

My suspicion is that either we are picking the wrong pointer out of the node list when dealing with multicopies, or perhaps when creating multicopies of FullPhysVols we do not correctly create an AlignableTransform for each of them. To be investigated...

Might be of interest to @jojungge @ncalace

OK, after some debugging I think I am a bit closer to an understanding of the issue.

• It seems to me that we never create a unique GeoAlignableTransform for each alignable element rather than just not finding the correct one. Running a test job, I see that for a multicopy of logvols which should all be alignable, we appear to call MakeTransformation::make and create a suitable transform with GeoIntrusivePtr gat = make_intrusive<GeoAlignableTransform>(hepTransform); only once. This means all of the copies get the same GeoAlignableTransform.

• The reason for this seems to be that the transformation which gets copied is in principle common to all the copies made, and share a common name. This means that the De-duplication in Element2GeoItem looks in its map to see if it already has a transformation with that name, and if it does, it just returns it rather than creating a new one:

GeoIntrusivePtr<RCBase> item{nullptr};
    EntryMap::iterator entry;
    if (name.empty()) { // Unnamed item; cannot store in the map; make a new one 
        item = make(element, gmxUtil);
    } else if ((entry = m_map.find(name)) == m_map.end()) { // Not in; make a new one
        item = make(element, gmxUtil);
        m_map[name] = item; // And put it in the map
    }
    else { // Get it from the map
        item = entry->second; 
    }

    return item;

Of course this is perfectly sensible in general, but for GeoAlignableTransforms which can be modified to differ after-the-fact, even if they are identical at construction time, this is not appropriate, and we should make sure we create a new object every time, like we do I believe for sensitive (or alignable) logvols. Clearly we do not want to remove the de-duplication altogether, so we need a new solution.

@rbianchi @jojungge do you have any thoughts on the best way to do this? My simple fix would be to add an optional boolean parameter to Element2GeoItem::process(const xercesc::DOMElement *element, GmxUtil &gmxUtil) making it Element2GeoItem::process(const xercesc::DOMElement *element, GmxUtil &gmxUtil, bool &allowDuplication=false) and having the logic be:

GeoIntrusivePtr<RCBase> item{nullptr};
    EntryMap::iterator entry;
    if (name.empty() || allowDuplication) { // Unnamed item or allowed to be duplicated; cannot store in the map; make a new one 
        item = make(element, gmxUtil);
    } else if ((entry = m_map.find(name)) == m_map.end()) { // Not in; make a new one
        item = make(element, gmxUtil);
        m_map[name] = item; // And put it in the map
    }
    else { // Get it from the map
        item = entry->second; 
    }

    return item;

Would this be OK? I don't see a reason to add the GeoAlignableTransforms to the map if we explicitly don't want to avoid duplicating them, but maybe I miss something. I guess if we do want to add them to the map we need to automatically add a suffix to the name or something?

Or would a more substantial (therefore also invasive) solution be preferred? I.e. something like making the AlignableTransforms separate from other Transforms so that we can toggle their de-duplication in the same way as other classes of objects?

mentioned in merge request !396 (closed)

mentioned in merge request !397 (merged)

It seems the previous fixes did not solve the problems 100%, since in some places we pass the alignable property to multicopies, which can create their own transformations, without going via the TransformProcesser::process method, which was updated to allow duplicates for AXF in !397 (merged):

map<string, GeoNodeList>::iterator entry;
    GeoNodeList *xfList;
    if ((entry = m_map.find(name)) == m_map.end()) { // Not in registry; make a new item
        //
        //    Add empty node list to the map
        //       
        xfList = &m_map[name];
        //
        //    Loopvar Variable name
        //
        string varname{}, firstElement{};
        XMLCh * loopvar_tmp = XMLString::transcode("loopvar");
        bool hasVarname = (element->getAttributeNode(loopvar_tmp) != 0);
        if (hasVarname) {
            toRelease = XMLString::transcode(element->getAttribute(loopvar_tmp));
            varname = toRelease;
            XMLString::release(&toRelease);
            XMLString::release(&loopvar_tmp);
            // Check it is a vector
            firstElement = varname + "_0";
            if (!gmxUtil.eval.findVariable(firstElement.c_str())) {
                THROW_EXCEPTION("Error in .gmx file. Processing multicopy element with name " << name << 
                ". Found loopvar set to " << varname << ", but no vector with that name has been defined.");
            }
        }
        //
        //    Get the transformation-element
        //
        DOMElement *elXf = element->getFirstElementChild();
        toRelease = XMLString::transcode(elXf->getNodeName());
        string nodeName(toRelease);
        XMLString::release(&toRelease);
        Element2GeoItem *xFormProcessor{nullptr};
        if (hasVarname) {
            if (nodeName == "transformation") { // OK
                xFormProcessor = &(gmxUtil.tagHandler.transformation);
            } else { // Not OK
                THROW_EXCEPTION("Error in .gmx file. Processing multicopy element with name " << name <<
                ". \nIt gives loopvar therefore should have a <transformation> and not a <transformationref> (despite the DTD)\n");
        
            }
        } else {
            xFormProcessor = nodeName == "transformation"?(Element2GeoItem*) &(gmxUtil.tagHandler.transformation): 
                                                          (Element2GeoItem*) &(gmxUtil.tagHandler.transformationref);
        }
        //
        //    Produce all the transformations
        //
        GeoIntrusivePtr<GeoAlignableTransform> geoAXf{nullptr};
        GeoIntrusivePtr<GeoTransform> geoXf{nullptr};
        if (hasVarname) {
            for (int i = 0; i < nCopies; ++i) {
                gmxUtil.eval.setVariable(varname.c_str(), (varname + "_" + to_string(i)).c_str());
                if (alignable) {
                    geoAXf = dynamic_pointer_cast<GeoAlignableTransform>(xFormProcessor->make(elXf, gmxUtil));
                    xfList->push_back(geoAXf);
                }
                else {
                    geoXf = dynamic_pointer_cast<GeoTransform>( xFormProcessor->make(elXf, gmxUtil));
                    xfList->push_back(geoXf);
                }
                gmxUtil.eval.removeVariable(varname.c_str()); // Avoids a warning status in evaluator
            }
        } else {
            //
            //    If varname not given, we get the CLHEP xForm and apply it (n-1) times sequentially to place the copies
            //    First copy gets the Identity transform (i.e. placed in "default position" for object)
            //
            //Is there a better way to just get the Transform3D?
            GeoTrf::Transform3D hepXf0  = dynamic_pointer_cast<GeoTransform>(xFormProcessor->make(elXf, gmxUtil))->getTransform();
            GeoTrf::Transform3D hepXf=GeoTrf::Transform3D::Identity(); // Identity initially for the first copy
            for (int i = 0; i < nCopies; ++i) {
                if (alignable){
                   geoAXf = make_intrusive<GeoAlignableTransform>(hepXf) ; 
                   xfList->push_back(geoAXf);
                }
                else{
                   geoXf = make_intrusive<GeoTransform>(hepXf) ; 
                   xfList->push_back(geoXf);
                }
                hepXf = hepXf0 * hepXf; //multiply by hepXf0 again for each copy
            }
        }
    } else {
        xfList = &entry->second;
    }

Specifically, you can see here that this code implements by itself a similar kind of de-duplication that was added in Element2GeoItem. Hopefully a simple addition like the following will solve the issue:

if ((entry = m_map.find(name)) == m_map.end() || alignable) { // Not in registry or explicitly should have a new set of transformation to allow unique alignment deltas; make a new item

mentioned in merge request !420 (merged)