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

add CellIDPositionConverter #155

Merged
merged 10 commits into from Jun 1, 2017

Conversation

Projects
None yet
3 participants
@gaede
Contributor

gaede commented May 31, 2017

Do not merge this PR yet !

For discussion:

  • I implemented the cellID to position conversion based in the discussions at the
    DD4hep mini-workshop w/o using any of the deprecated members in the VolumeManager
  • now also implemented a first version of the cellID(position) lookup that seems to (mostly work)

BEGINRELEASENOTES

  • add new class DDRec::CellIDPositionConverter
    - replaces DDRec::IDDecoder
    - implement positionNominal(CellID id) and cellID(position)
    • prepare for using alignment map by separating transforms to DetElement and daughter volume
    • do not use deprecated methods/members in VolumeManager
  • add test_cellid_position_converter.cpp
  • add VolumeManagerContext::toElement
    • transform from sensitive volume to next DetElement

ENDRELEASENOTES

@petricm petricm changed the title from add CellIDPositionConverter to [WIP] add CellIDPositionConverter May 31, 2017

@petricm

This comment has been minimized.

Member

petricm commented Jun 1, 2017

I think on one meeting we concluded that #131 , does this meet this requirement?

@gaede

This comment has been minimized.

Contributor

gaede commented Jun 1, 2017

@petricm yes, this PR fixes #131, i.e. the lookup of the position for a given sensitive cellID works also in setups where there is no DetElement directly assigned to the sensitive volume.

@gaede gaede requested a review from MarkusFrankATcernch Jun 1, 2017

@@ -83,6 +83,7 @@ namespace DD4hep {

This comment has been minimized.

@petricm

petricm Jun 1, 2017

Member

Did you forget to stage? This commit is just a new line.

This comment has been minimized.

@gaede

gaede Jun 1, 2017

Contributor

@petricm read the commit message ;-)

This comment has been minimized.

@petricm

petricm Jun 1, 2017

Member

Oh, I understand this is git commit --amend

This comment has been minimized.

@gaede

gaede Jun 1, 2017

Contributor

yes, but you are not supposed to use this after a push ...

This comment has been minimized.

@andresailer

andresailer Jun 1, 2017

Member

It is ok to use it as long as your branch is not merged. Those two commits shoud be squashed.
And you can fix the typo in the next commit message

@gaede

This comment has been minimized.

Contributor

gaede commented Jun 1, 2017

@MarkusFrankATcernch could you please have a look at the implementation of the cellID<->position lookups if this is the way it should work ?
This is without the alignment yet, but the transformations are already split between the DetElement worldTrafo and the sensitive volume to DetElement transformantion, so that we can easily replace the det.nominal() with the corect one, once the AlignementMap will be available...

gaede added some commits Jun 1, 2017

fix computation of cellID from postion
  - combine DetElement volID with volIDs of placed daughters
  - comment out debug statements

@petricm petricm merged commit d7915b9 into AIDASoft:master Jun 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gaede gaede deleted the gaede:cellid_position_converter branch Jun 1, 2017

@petricm petricm changed the title from [WIP] add CellIDPositionConverter to add CellIDPositionConverter Jun 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment