Skip to content
Snippets Groups Projects

Add support for more complicated mutlicopies in alignment

Merged Nicholas Styles requested to merge nstyles/GeoModel:multicopyUpdates into main
All threads resolved!

In the process of developing ITk alignment, it was observed that some multicopies caused issues when it was attempted to add the alignable property. This turned out to be because if more than one transform is defined within a multicopy, the assumed location of various items in the vector of objects to process is incorrect.

To handle this, a more robust approach is taken by actively looking for the required objects in the vector, to use the correct one if found. While this is a bit less efficient, this code is anyway not performance critical as it will only run once per job (and is anyway still pretty fast).

Not addressed here, but to be followed up in later MRs:

  • The behavior of multicopy when a loopvar is not used does not seem correct regarding alignment. Following up with the original author
  • it can be confusing that alignable can be both a bool (to indicate that a volume should be a FullPhysVol and a transform an AlignableTransform) and an int (to indicate that addAlignment should be called, with a "level" indicated by the int to define at which level in the geometry hierarchy it should be considered when running alignment). A new property such as alignLevel should be added which initially duplicates the int version of alignable while the geometry versions are being moved over to the new syntax. Eventually, the handling old int property could be removed from the code (so it will be ignored if defined) and from newer versions of the .dtd file.

FYI @xilin @sroe

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Any objections to this @boudreau @tsulaia @jojungge? I have another update lined up after this one, so it would be good if this could go in, unless you have objections?

  • Nicholas Styles resolved all threads

    resolved all threads

  • Nicholas Styles resolved all threads

    resolved all threads

  • Nicholas Styles added 1 commit

    added 1 commit

    • b2360141 - Apply 2 suggestion(s) to 1 file(s)

    Compare with previous version

  • Looks generally good to me :)

    • Resolved by Nicholas Styles

      Deferring to Johannes. Since it looks good to him I will merge.

      @nstyles, we are planning a documentation week, April 8. There has been a large amount of development in GeoModelXML and you are promising even more. Can you consider joining the documentation effort, either during our dedicated week or asynchronously?

  • Joseph Boudreau resolved all threads

    resolved all threads

  • Joseph Boudreau enabled an automatic merge when the pipeline for b2360141 succeeds

    enabled an automatic merge when the pipeline for b2360141 succeeds

  • Nicholas Styles added 1 commit

    added 1 commit

    • 2012da6e - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

    • Resolved by Vakhtang Tsulaia
      ================================================================================
                         Rebase the GeoModel code base w.r.t. main
      ================================================================================
      CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC
      fatal: ambiguous argument 'origin/main': unknown revision or path not in the working tree.
      Use '--' to separate paths from revisions, like this:
      'git <command> [<revision>...] -- [<file>...]'

      Looks like a problem in the test definition? Ambiguity between Athena/main and GeoModel/main perhaps?

  • Nicholas Styles resolved all threads

    resolved all threads

  • Vakhtang Tsulaia resolved all threads

    resolved all threads

  • All looks good now. Merging

  • Vakhtang Tsulaia mentioned in commit bdebbf0a

    mentioned in commit bdebbf0a

  • Nicholas Styles mentioned in merge request !311 (merged)

    mentioned in merge request !311 (merged)

  • Please register or sign in to reply
    Loading