Error in testDD4hepFilteredViewLevel and testDD4hepFilteredViewGoTo

[silviodonato]

We are getting two errors in the unit test of DetectorDescription/DDCMS in testDD4hepFilteredViewLevel and testDD4hepFilteredViewGoTo since CMSSW_11_2_X_2020-09-27-0000, after having merged #31508.
It turned out that the error is related to the hard-coded level numbers in the unit test (eg. https://github.com/cms-sw/cmssw/blob/master/DetectorDescription/DDCMS/test/DDFilteredView.goto.cppunit.cc#L73).
More details in #31508.
@ianna could you have a look to this issue?

[silviodonato]

assign geometry

[cmsbuild]

New categories assigned: geometry

@Dr15Jones,@cvuosalo,@mdhildreth,@makortel,@ianna,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

A new Issue was created by @silviodonato Silvio Donato.

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[ianna]

@silviodonato - I do not see errors in the PR IB linked here. I'll check in the local area.

[qliphy]

@ianna You can find more info from #31574
#31574 (comment)

[ianna]

@qliphy - thanks! I've reproduced the error in a local area. The scenario has changed. I'm afraid, the construction order is not guaranteed, so the reference point has shifted.

[ianna]

please, take #31605 that fixes it.

[cvuosalo]

@ianna Can this problem happen again? How can we prevent it?

[ianna]

+1

[cmsbuild]

This issue is fully signed and ready to be closed.

[ianna]

@ianna Can this problem happen again? How can we prevent it?

Good question. Yes, it will definitely happen when the geometry changes: a geo history reflects a child number at a level, but if there is another child created before, the geo history of the first one will shift.

Will it happen if geometry does not change? Yes, when we will start using multiple processes. We cannot guarantee the order of parsing and creation.

[ianna]

Here is an explanation why the unit test failed: two copies of a ZDCtoFP4201 and a ZDCtoFP4202 volumes were added shifting the Tracker from the 4th to the 6th position.
Here is the snapshot of a 2021 geometry tree in the IB:

where it used to be only two copies of a ZDCtoFP420 volume:

[ianna]

@silviodonato - IMHO, this can be closed now. Thanks!

[silviodonato]

@ianna I would keep this issue open to remind us that the tests are currently incompatible with geometry changes and multiple processes. If you prefer we can close this issue and open another one. Of course this issue is much less urgent than the DD4HEP migration, but I think it is good to keep a reminder.

[ianna]

@ianna I would keep this issue open to remind us that the tests are currently incompatible with geometry changes and multiple processes. If you prefer we can close this issue and open another one. Of course this issue is much less urgent than the DD4HEP migration, but I think it is good to keep a reminder.

@silviodonato - the unit test does exactly what it is suppose to do - it fails if the geometry changes ;-)

[cvuosalo]

@smuzaffar Could the unit test described in this issue be added to the PR tests for any change to "Geometry" packages?

[silviodonato]

Could the unit test described in this issue be added to the PR tests for any change to "Geometry" packages?

@cvuosalo you can include manually this unit test by adding DetectorDescription/DDCMS in the PR test (see http://cms-sw.github.io/cms-bot-cmssw-cmds.html)

test parameters:
addpkg = DetectorDescription/DDCMS

[silviodonato]

@ianna I would keep this issue open to remind us that the tests are currently incompatible with geometry changes and multiple processes. If you prefer we can close this issue and open another one. Of course this issue is much less urgent than the DD4HEP migration, but I think it is good to keep a reminder.

@silviodonato - the unit test does exactly what it is suppose to do - it fails if the geometry changes ;-)

Ok, so I assume that you want to keep the test as it is. If somebody wants to change the geometry, s/he needs also to update the unit test. As @cvuosalo pointed out, it would be good to run this test anytime we change a Geometry package