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

DetectorData: Warn if a second object (e.g., constant) is created, as… #200

Merged
merged 1 commit into from Jul 6, 2017

Conversation

Projects
None yet
4 participants
@andresailer
Member

andresailer commented Jul 4, 2017

… insert will not overwrite the existing one

BEGINRELEASENOTES

  • Now will give a warning if multiple entities (e.g., constants) of the same name are defined in the XML

ENDRELEASENOTES

DetectorData: Warn if a second object (e.g., constant) is created, as…
… insert will not overwrite the existing one
@andresailer

This comment has been minimized.

Member

andresailer commented Jul 5, 2017

Tests fail because of #199

@gaede

This comment has been minimized.

Contributor

gaede commented Jul 5, 2017

Can you or someone else please look into why the tests fail - my commit was a necessary and obvious bug fix - so I assume that sth. else is broken there...

@andresailer

This comment has been minimized.

Member

andresailer commented Jul 5, 2017

Some transformation matrices are no longer what is expected by the tests, maybe your obvious bugfix broke something else?
https://travis-ci.org/AIDASoft/DD4hep/builds/249979476

@gaede

This comment has been minimized.

Contributor

gaede commented Jul 5, 2017

Seems to me that there is indeed an additional bug somewhere (VolumeMgr, Alignment ?), based on the following output:

33: Placement [4]  VolID:sensor=0001 		matrix component1_placement - tr=1  rot=0  refl=0  scl=0
33:   1.000000    0.000000    0.000000    Tx =   0.000000
33:   0.000000    1.000000    0.000000    Ty =   0.000000
33:   0.000000    0.000000    1.000000    Tz =   0.114500
33: Placement [3]  VolID:module=002f 		matrix SiTrackerModule_Layer1_placement - tr=1  rot=1  refl=0  scl=0
33:  -0.984716   -0.000000    0.174167    Tx =   0.069667
33:  -0.174167   -0.000000   -0.984716    Ty = -22.529386
33:   0.000000   -1.000000    0.000000    Tz =   8.535467
33: Placement [2]  VolID:layer=0001 		matrix  - tr=0  rot=0  refl=0  scl=0
33:   1.000000    0.000000    0.000000    Tx =   0.000000
33:   0.000000    1.000000    0.000000    Ty =   0.000000
33:   0.000000    0.000000    1.000000    Tz =   0.000000
33: Placement [1]  VolID:system=00ff barrel=0000 		matrix  - tr=0  rot=0  refl=0  scl=0
33:   1.000000    0.000000    0.000000    Tx =   0.000000
33:   0.000000    1.000000    0.000000    Ty =   0.000000
33:   0.000000    0.000000    1.000000    Tz =   0.000000
33: Computed Trafo (from placements):		matrix  - tr=1  rot=1  refl=0  scl=0
33:  -0.984716   -0.000000    0.174167    Tx =   0.089609
33:  -0.174167   -0.000000   -0.984716    Ty = -22.642136
33:   0.000000   -1.000000    0.000000    Tz =   8.535467
33: DetElement Trafo: /world/SiTrackerBarrel/layer1 [00000008000000ff]		matrix  - tr=1  rot=1  refl=0  scl=0
33:   1.000000    0.000000    0.000000    Tx =   0.000000
33:   0.000000    1.000000    0.000000    Ty =   0.000000
33:   0.000000    0.000000    1.000000    Tz =   0.000000
33: VolumeMgr  Trafo: /world/SiTrackerBarrel/layer1 [002005e8000000ff]		matrix  - tr=1  rot=1  refl=0  scl=0
33:   1.000000    0.000000    0.000000    Tx =   0.000000
33:   0.000000    1.000000    0.000000    Ty =   0.000000
33:   0.000000    0.000000    1.000000    Tz =   0.000000

so the world transformation returned by the volume manager is incorrect, as it is missing the one placement [3] that is different from the identity.
It seems to me that the problem is rather in the Alignment, as this misses this transformation as well. No idea how to fix this.

@MarkusFrankATcernch can you please, please have a look at the problem ?

Additionally, looking at this line:
https://github.com/AIDASoft/DD4hep/blob/master/DDCore/src/VolumeManager.cpp#L272
should this not rather read:

ext->placement  = PlacedVolume( nodes[nodes.size()-1] );  // store the placement of last node in the chain !! ?

I tried this, but the test still fails.

PS:
Just to clarify again: my fix from yesterday is needed. Only with this do I get the right context for volumes that are in the DetElement's sub-hierarchy and all the cellId to position lookups for ILD calorimeters work again.

@andresailer

This comment has been minimized.

Member

andresailer commented Jul 5, 2017

Just to emphasise: don't merge things that break tests. Maybe your fix is needed, maybe something else is broken in DD4hep, maybe your fix only fixes one thing but breaks another.

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Jul 5, 2017

Which test is creating this output?

@andresailer

This comment has been minimized.

Member

andresailer commented Jul 5, 2017

One of those:

The following tests FAILED:
	 33 - t_ClientTests_MultiPlace (Failed)
	 34 - t_ClientTests_Bitfield64_LongVoldID (Failed)
Errors while running CTest

See https://travis-ci.org/AIDASoft/DD4hep/builds/249979476 for log file

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Jul 5, 2017

https://github.com/AIDASoft/DD4hep/blob/master/DDCore/src/VolumeManager.cpp#L272
should this not rather read:
ext->placement = PlacedVolume( nodes[nodes.size()-1] ); // store the placement of last node in the chain !! ?

These 2 statements are identical. Have a look in scanPhysicalVolume.
The node is pushed to the back of the chain before the call to add_entry.

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Jul 5, 2017

This output is very odd. I get the following:

SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1/module2/component1                    vid:00200048000000ff system:00ff barrel:0000 layer:0001 module:0002 sensor:0001 Parent-OK:YES
Placement [4]  VolID:sensor=0001 		matrix component1_placement - tr=1  rot=0  refl=0  scl=0
  1.000000    0.000000    0.000000    Tx =   0.000000
  0.000000    1.000000    0.000000    Ty =   0.000000
  0.000000    0.000000    1.000000    Tz =   0.114500
Placement [3]  VolID:module=0002 		matrix SiTrackerModule_Layer1_placement - tr=1  rot=1  refl=0  scl=0
  0.174167    0.000000    0.984716    Tx =  22.529386
 -0.984716    0.000000    0.174167    Ty =   0.069667
 -0.000000   -1.000000    0.000000    Tz = -42.677333
Placement [2]  VolID:layer=0001 		matrix  - tr=0  rot=0  refl=0  scl=0
  1.000000    0.000000    0.000000    Tx =   0.000000
  0.000000    1.000000    0.000000    Ty =   0.000000
  0.000000    0.000000    1.000000    Tz =   0.000000
Placement [1]  VolID:system=00ff barrel=0000 		matrix  - tr=0  rot=0  refl=0  scl=0
  1.000000    0.000000    0.000000    Tx =   0.000000
  0.000000    1.000000    0.000000    Ty =   0.000000
  0.000000    0.000000    1.000000    Tz =   0.000000
Computed Trafo (from placements):		matrix  - tr=1  rot=1  refl=0  scl=0
  0.174167    0.000000    0.984716    Tx =  22.642136
 -0.984716    0.000000    0.174167    Ty =   0.089609
 -0.000000   -1.000000    0.000000    Tz = -42.677333
DetElement Trafo: /world/SiTrackerBarrel/layer1/module2/component1 [00200048000000ff]		matrix  - tr=1  rot=1  refl=0  scl=0
  0.174167    0.000000    0.984716    Tx =  22.642136
 -0.984716    0.000000    0.174167    Ty =   0.089609
 -0.000000   -1.000000    0.000000    Tz = -42.677333
VolumeMgr  Trafo: /world/SiTrackerBarrel/layer1/module2/component1 [00200048000000ff]		matrix  - tr=1  rot=1  refl=0  scl=0
  0.174167    0.000000    0.984716    Tx =  22.642136
 -0.984716    0.000000    0.174167    Ty =   0.089609
 -0.000000   -1.000000    0.000000    Tz = -42.677333
SiTrackerBarrel  ERROR DETELEMENT_PLACEMENT: PASSED. All matrices equal: 00200048000000ff

To me this output looks OK. Or am I wrong here?
So what is really going wrong here?

@gaede

This comment has been minimized.

Contributor

gaede commented Jul 5, 2017

If you run

ctest -V -R t_ClientTests_MultiPlace

you should see the following somewhere in the output (note it is the VOLUME_PLACEMENT FAILED - not the DETELEMENT_PLACEMENT):

33: SiTrackerBarrel  INFO  Volume:component2_2                                                  [N]  vid:00000648000000ff system:00ff barrel:0000 layer:0001 module:0032 sensor:0000
33: SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1/module50                              vid:00000648000000ff system:00ff barrel:0000 layer:0001 module:0032 sensor:0000 Parent-OK:YES
33: SiTrackerBarrel  INFO  Volume:component3_3                                                  [N]  vid:00000648000000ff system:00ff barrel:0000 layer:0001 module:0032 sensor:0000
33: SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1/module50                              vid:00000648000000ff system:00ff barrel:0000 layer:0001 module:0032 sensor:0000 Parent-OK:YES
33: SiTrackerBarrel  INFO  Volume:SiTrackerModule_Layer1_50                                     [N]  vid:00000668000000ff system:00ff barrel:0000 layer:0001 module:0033 sensor:0000
33: SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1                                       vid:00000008000000ff system:00ff barrel:0000 layer:0001 module:0000 sensor:0000 Parent-OK:YES
33: SiTrackerBarrel  INFO  Volume:component0_0                                                  [N]  vid:00000668000000ff system:00ff barrel:0000 layer:0001 module:0033 sensor:0000
33: SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1                                       vid:00000008000000ff system:00ff barrel:0000 layer:0001 module:0000 sensor:0000 Parent-OK:YES
33: SiTrackerBarrel  INFO  Volume:component1_1                                       IDDesc:OK  [S]  vid:00200668000000ff system:00ff barrel:0000 layer:0001 module:0033 sensor:0001
33: SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1                                       vid:00000008000000ff system:00ff barrel:0000 layer:0001 module:0000 sensor:0000 Parent-OK:YES
33: Placement [4]  VolID:sensor=0001 		matrix component1_placement - tr=1  rot=0  refl=0  scl=0
33:   1.000000    0.000000    0.000000    Tx =   0.000000
33:   0.000000    1.000000    0.000000    Ty =   0.000000
33:   0.000000    0.000000    1.000000    Tz =   0.114500
33: Placement [3]  VolID:module=0033 		matrix SiTrackerModule_Layer1_placement - tr=1  rot=1  refl=0  scl=0
33:  -0.984716   -0.000000    0.174167    Tx =   0.069667
33:  -0.174167   -0.000000   -0.984716    Ty = -22.529386
33:   0.000000   -1.000000    0.000000    Tz =  42.677333
33: Placement [2]  VolID:layer=0001 		matrix  - tr=0  rot=0  refl=0  scl=0
33:   1.000000    0.000000    0.000000    Tx =   0.000000
33:   0.000000    1.000000    0.000000    Ty =   0.000000
33:   0.000000    0.000000    1.000000    Tz =   0.000000
33: Placement [1]  VolID:system=00ff barrel=0000 		matrix  - tr=0  rot=0  refl=0  scl=0
33:   1.000000    0.000000    0.000000    Tx =   0.000000
33:   0.000000    1.000000    0.000000    Ty =   0.000000
33:   0.000000    0.000000    1.000000    Tz =   0.000000
33: Computed Trafo (from placements):		matrix  - tr=1  rot=1  refl=0  scl=0
33:  -0.984716   -0.000000    0.174167    Tx =   0.089609
33:  -0.174167   -0.000000   -0.984716    Ty = -22.642136
33:   0.000000   -1.000000    0.000000    Tz =  42.677333
33: DetElement Trafo: /world/SiTrackerBarrel/layer1 [00000008000000ff]		matrix  - tr=1  rot=1  refl=0  scl=0
33:   1.000000    0.000000    0.000000    Tx =   0.000000
33:   0.000000    1.000000    0.000000    Ty =   0.000000
33:   0.000000    0.000000    1.000000    Tz =   0.000000
33: VolumeMgr  Trafo: /world/SiTrackerBarrel/layer1 [00200668000000ff]		matrix  - tr=1  rot=1  refl=0  scl=0
33:   1.000000    0.000000    0.000000    Tx =   0.000000
33:   0.000000    1.000000    0.000000    Ty =   0.000000
33:   0.000000    0.000000    1.000000    Tz =   0.000000
33: SiTrackerBarrel  ERROR VOLUME_PLACEMENT FAILED: World transformation DIFFER.

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Jul 5, 2017

I do NOT see this output. I get the following one, which looks correct:

SiTrackerBarrel  INFO  Volume:component2_2                                                  [N]  vid:00000628000000ff system:00ff barrel:0000 layer:0001 module:0031 sensor:0000
SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1                                       vid:00000008000000ff system:00ff barrel:0000 layer:0001 module:0000 sensor:0000 Parent-OK:YES
SiTrackerBarrel  INFO  Volume:component3_3                                                  [N]  vid:00000628000000ff system:00ff barrel:0000 layer:0001 module:0031 sensor:0000
SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1                                       vid:00000008000000ff system:00ff barrel:0000 layer:0001 module:0000 sensor:0000 Parent-OK:YES
SiTrackerBarrel  INFO  Volume:SiTrackerModule_Layer1_49                                     [N]  vid:00000648000000ff system:00ff barrel:0000 layer:0001 module:0032 sensor:0000
SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1/module50                              vid:00000648000000ff system:00ff barrel:0000 layer:0001 module:0032 sensor:0000 Parent-OK:YES
SiTrackerBarrel  INFO  Volume:component0_0                                                  [N]  vid:00000648000000ff system:00ff barrel:0000 layer:0001 module:0032 sensor:0000
SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1/module50                              vid:00000648000000ff system:00ff barrel:0000 layer:0001 module:0032 sensor:0000 Parent-OK:YES
SiTrackerBarrel  INFO  Volume:component1_1                                       IDDesc:OK  [S]  vid:00200648000000ff system:00ff barrel:0000 layer:0001 module:0032 sensor:0001
SiTrackerBarrel  INFO    Elt:/world/SiTrackerBarrel/layer1/module50/component1                   vid:00200648000000ff system:00ff barrel:0000 layer:0001 module:0032 sensor:0001 Parent-OK:YES
Placement [4]  VolID:sensor=0001 		matrix component1_placement - tr=1  rot=0  refl=0  scl=0
  1.000000    0.000000    0.000000    Tx =   0.000000
  0.000000    1.000000    0.000000    Ty =   0.000000
  0.000000    0.000000    1.000000    Tz =   0.114500
Placement [3]  VolID:module=0032 		matrix SiTrackerModule_Layer1_placement - tr=1  rot=1  refl=0  scl=0
 -0.984716   -0.000000    0.174167    Tx =  -0.000000
 -0.174167   -0.000000   -0.984716    Ty = -22.135500
  0.000000   -1.000000    0.000000    Tz =  34.141867
Placement [2]  VolID:layer=0001 		matrix  - tr=0  rot=0  refl=0  scl=0
  1.000000    0.000000    0.000000    Tx =   0.000000
  0.000000    1.000000    0.000000    Ty =   0.000000
  0.000000    0.000000    1.000000    Tz =   0.000000
Placement [1]  VolID:system=00ff barrel=0000 		matrix  - tr=0  rot=0  refl=0  scl=0
  1.000000    0.000000    0.000000    Tx =   0.000000
  0.000000    1.000000    0.000000    Ty =   0.000000
  0.000000    0.000000    1.000000    Tz =   0.000000
Computed Trafo (from placements):		matrix  - tr=1  rot=1  refl=0  scl=0
 -0.984716   -0.000000    0.174167    Tx =   0.019942
 -0.174167   -0.000000   -0.984716    Ty = -22.248250
  0.000000   -1.000000    0.000000    Tz =  34.141867
DetElement Trafo: /world/SiTrackerBarrel/layer1/module50/component1 [00200648000000ff]		matrix  - tr=1  rot=1  refl=0  scl=0
 -0.984716   -0.000000    0.174167    Tx =   0.019942
 -0.174167   -0.000000   -0.984716    Ty = -22.248250
  0.000000   -1.000000    0.000000    Tz =  34.141867
VolumeMgr  Trafo: /world/SiTrackerBarrel/layer1/module50/component1 [00200648000000ff]		matrix  - tr=1  rot=1  refl=0  scl=0
 -0.984716   -0.000000    0.174167    Tx =   0.019942
 -0.174167   -0.000000   -0.984716    Ty = -22.248250
  0.000000   -1.000000    0.000000    Tz =  34.141867
SiTrackerBarrel  ERROR DETELEMENT_PLACEMENT: PASSED. All matrices equal: 00200648000000ff

So what are you doing different?

@gaede

This comment has been minimized.

Contributor

gaede commented Jul 5, 2017

This is strange indeed. Does your test pass, when running

ctest -V -R t_ClientTests_MultiPlace  

For me it fails on macosx with llvm - will try also on SL6 ...

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Jul 5, 2017

Yes. All tests pass. They also all passed on gitlab before my last commit....

@andresailer

This comment has been minimized.

Member

andresailer commented Jul 5, 2017

Markus, did you pull from the master? Did you push the relevant commit to gitlab? I don't see that in your gitlab fork.
Gitlab mirror of dd4hep is all red https://gitlab.cern.ch/CLICdp/DetectorSoftware/DD4hep/pipelines/155594

Also this whole discussion has nothing to do with this PR and should go to the other PR #199 or a new issue

@gaede

This comment has been minimized.

Contributor

gaede commented Jul 5, 2017

Markus, this fixes the test for me:

/// Acces the sensitive volume placement
PlacedVolume VolumeManagerContext::placement()  const   {
  return element.placement() ;
//  return (0 == flag ) ? element.placement() : _getExtension(this)->placement;
}

Not sure though, if this is what you intended to return in this method, the placement of the element or sensitive volume ...

@gaede

This comment has been minimized.

Contributor

gaede commented Jul 5, 2017

... with the above mentioned fix, all tests pass for me:

100% tests passed, 0 tests failed out of 126

So this would at least make our guardians of public moral happy.

However, we seem to have a problem with the API and or its documentation then .

In a previous version of DD4hep VolumeManagerContext::placement() was marked deprecated, now it isn't anymore - and what should it really return ?

@andresailer

This comment has been minimized.

Member

andresailer commented Jul 5, 2017

guardians of public moral
Seriously?

@MarkusFrankATcernch

This comment has been minimized.

Contributor

MarkusFrankATcernch commented Jul 6, 2017

If it fixes the problem, we can accept the pull for now.
This fix however will break the VolumeManager for sensitive volumes,
which are not the placement of a DetElement.

@gaede

This comment has been minimized.

Contributor

gaede commented Jul 6, 2017

The problem this fixes is a failed test - at the cost of being obviously wrong, or at least inconsistent and most likely hiding a problem somewhere else (the Alignment ?).
The cellId to position conversion works also for sensitive volumes that are not the placement of a DetElement if going through the VolumeManagerContext::toElement, as is done here:
https://github.com/AIDASoft/DD4hep/blob/master/DDRec/src/CellIDPositionConverter.cpp#L27-L70
(and as we had discussed in one of our last developers meeting.)
We need to discuss again at some point, what to do with VolumeManagerContext::placement.
The iLCSoft simulation and reconstruction chain works with or without this fix.
I have no strong opinion on whether it is better to have a failed test that reminds us that sth. needs attention or have a workaround committed, marked as FIXME:

/// Acces the sensitive volume placement
PlacedVolume VolumeManagerContext::placement()  const   {
//FIXME: this temporarily fixes a test with alignment transformations but needs revisiting
  return element.placement() ;
//  return (0 == flag ) ? element.placement() : _getExtension(this)->placement;
}

I leave it to Andre to decide, if he wants to add this to this PR before merging it.

Andre, if you want this PR to go into the iLCSoft release, I need a new tag before lunch. Otherwise I will use v01-00-01.

@petricm

This comment has been minimized.

Member

petricm commented Jul 6, 2017

I am against merging "obviously wrong"\cite{@gaede} code. But if someone insists the FIXME should be a #pragma message, otherwise this will be forgotten.

@andresailer

This comment has been minimized.

Member

andresailer commented Jul 6, 2017

As I have written repeatedly, this PR has nothing to do with the failing test or transformations, so I will just merge it. For the transformation issue we need further clarifications.

@andresailer andresailer merged commit ea505c5 into AIDASoft:master Jul 6, 2017

1 check failed

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

@andresailer andresailer deleted the andresailer:warnDuplicates branch Jul 6, 2017

MarkusFrankATcernch added a commit to MarkusFrankATcernch/DD4hep that referenced this pull request Jul 6, 2017

MarkusFrankATcernch added a commit that referenced this pull request Jul 6, 2017

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