Project

General

Profile

Bug #18507

Geometry tests are failing for protDUNE SP

Added by David Adams about 3 years ago. Updated about 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Start date:
12/08/2017
Due date:
% Done:

0%

Estimated time:
Duration:

Description

Christoph and Marting report the following:

Hi Martin,

CC: David and Gianluca

The unit test for the new protoDUNE SP geometry (v4) that you have pushed yesterday fails at the space point check:

716: test_GeometryDune: Check 54 space points.
717:   ( -220.554,  85.6294,  117.526): C:0 T:1
718:     TPC  1 plane 0 nearest wire is 862 and coordinate is 1189.45
719:
720: ---------------------------------
721: checkfloat: 1189.45 != 1181.6
722: ---------------------------------
723: test_GeometryProtoDune: /scratch/workspace/dune_ci_slf/label_exp/SLF6/label_exp2/swarm/DUNE/srcs/dunetpc/dune/Geometry/test/test_GeometryDune.cxx:135: void {anonymous}::checkfloat(double, double, double): Assertion `false' failed.
724: <end of output>
725: Test time =  49.28 sec
726: ----------------------------------------------------------
727: Test Failed.

- Full test log: http://dbweb5.fnal.gov:8080/LarCI/app/ns:dune/storage/docs/2017/12/07/test_GeometryProtoDune_2WvUqJK.log

History

#1 Updated by David Adams about 3 years ago

  • Subject changed from 35t reco is missing tools in fcl to Geometry tests are failing for protDUNE SP

Fix title (I was confusing my issues)

#2 Updated by David Adams about 3 years ago

The test was written a while ago and this reply depends on my recollection. Points were somehow selected and the test run to get expected results and then results hardwired. The assumption was that the physical geometry would not change and we want to test changes in the larsoft and DUNE geometry interpretation code.

If the detector elements are moving (wrt to the coordinate system), then we will need to update the test.

We would like to keep dunetpc develop in a state where it always builds and tests successfully. Please build and run tests before committing changes. I have just confirmed in my build area that the test test_GeometryProtoDune fails. All else is OK.

We should quickly 1. revert the geometry change, 2. drop the test or 3. fix the test. Maybe do 2 while we work on 3. Martin, will you this or should I?

da

#3 Updated by David Adams about 3 years ago

Martin:

We would like to cut release v06_59_00 without any test failures. Do we want to keep this geometry change? Should we simply skip this test for that release?

#4 Updated by Gianluca Petrillo about 3 years ago

  • Description updated (diff)

#5 Updated by David Adams about 3 years ago

Martin has privately suggested we revert to v3 of the geometry for the imminent release.

Martin, can you make this change change or provide instructions on exactly what needs to be changed?

#6 Updated by David Adams about 3 years ago

From Martin:
-----
OK, I have changed the fhicl files to use geometry v3 by default.

Or should I remove v4 from develop?
-----

From me:

Martin, I think it is OK to leave the leave the (unused) v4 from develop. But, as you wish.

Are the changes committed.?

#7 Updated by David Adams about 3 years ago

It looks like the changes are in develop. I will start building from this version.

#8 Updated by Dorota Stefan about 3 years ago

Dear All,

This geometry change is one of the important thing which we would like to run in MCC10 for ProtoDUNE-SP. Fortunately, we decided to move MCC10 for the next LArSoft release. It means, that we would like to have geo v4 from Martin ready in dunetpc v06_60_00.

I have tested geometry from Martin and everything was working for me but I did not run detailed tests. Is it possible that that the geometry test just needs updates? Are there other serious problems?

Please, let me know,
Dorota

#9 Updated by David Adams about 3 years ago

It sounds like you are confident the v4 geometry is OK. If you wish I can update the expected results for the test to match the current results.

#10 Updated by Dorota Stefan about 3 years ago

For me all is working but I made very basic checks. All details of the geometry knows Martin. Martin do you see something that worries you?

Many thanks,
Dorota

#11 Updated by Dorota Stefan about 3 years ago

Hi David,

I am confident with my tests. May I ask you to update the expected results for the test to match the current results? I am sure we are going to have further improvements in the protodune geometry, but for MCC10 we need the current version of the geometry, version 4.

#12 Updated by Tingjun Yang about 3 years ago

I think the first step is for Martin to change dunetpc to use v4 geometry. David can then update the CI test.

#13 Updated by David Adams about 3 years ago

OK, I am working on this now. --da

#14 Updated by David Adams about 3 years ago

For fixing the test, is it enough for me to change v3 to v4 in these lines

protodune_geo.GDML: "protodune_v3.gdml" 
protodune_geo.ROOT: "protodune_v3.gdml" 

in geometry_dune.fcl?

I see Martin had other changes but maybe they don't affect the test???

I get the original error when I make this change.

#15 Updated by Tingjun Yang about 3 years ago

Martin had several changes in his 8059bc25589f6806bc92fd19586ef53eb93e1173
Not sure if the other two changes need to be reverted as well.

#16 Updated by Martin Tzanov about 3 years ago

Yes, changing v3 to v4 in the geometry_dune.fcl is one of the changes.
In addition, the starting point should modified in the gensingle fhicl file
to make sure the particles start on the beamline axis.

We need to understand the reason for the failure. I have not changed anything
inside the TPC geometry. In other words if it works for v3 it should work
for v4.

Are the coordinates printed by the test in cm or in mm?

#17 Updated by David Adams about 3 years ago

The test takes units from the LArSoft geometry. It reports:

CryostatHalfWidth: 427.6
CryostatHalfHeight: 395.2
CryostatLength: 855.2

and so I suppose these are cm.

The space points used to test the detector are based on the cryostat limits. If the cryostat has moved or changed size, then the space points will change.

Here is what I see for v4:

CryostatHalfWidth: 427.52
CryostatHalfHeight: 395.12
CryostatLength: 855.04

A little different.

#18 Updated by David Adams about 3 years ago

I have generated new expected test results and put them in dunetpc/dune/Geometry/test/setProtoDuneSpacePoints_v4.dat. For the record, these are generated automatically for any geometry by running with its data file (here setProtoDuneSpacePoint.dat) empty. I have copied the old test values to setProtoDuneSpacePoints_v3.dat.

Here is the first point for v3:

  ev.posXyz.push_back(SpacePoint(-220.554, 85.6294, 117.526));
  ev.posTpc.push_back(1);
  ev.posPla.push_back(0);
  ev.posWco.push_back(1181.6);
  ev.posTpc.push_back(1);
  ev.posPla.push_back(1);
  ev.posWco.push_back(1284.44);
  ev.posTpc.push_back(1);
  ev.posPla.push_back(2);
  ev.posWco.push_back(177.523);

and v4:
  ev.posXyz.push_back(SpacePoint(-220.506, 85.6774, 117.574));
  ev.posTpc.push_back(1);
  ev.posPla.push_back(0);
  ev.posWco.push_back(1189.47);
  ev.posTpc.push_back(1);
  ev.posPla.push_back(1);
  ev.posWco.push_back(1276.45);
  ev.posTpc.push_back(1);
  ev.posPla.push_back(2);
  ev.posWco.push_back(187.013);

The posWco values are wire coordinates (i.e. units are wire spacing). There is a small change (< 1 mm) in the position of the test point but the coordinates of the nearest wires change by 8-10 wires. Is this inconsistent with your expectations?

I would like to commit the v3 and v4 files. Note these are not used in the testing. Please let me know if I should also commit the update to v4 geometry and test data.

#19 Updated by David Adams about 3 years ago

  • Status changed from Assigned to Closed

I did not hear back on the above and so I have switched back to the v3 geometry. As discussed above, test data files for both v3 and v4 are in dunetpc/dune/Geometry/test. Just copy setProtoDuneSpacePoints_v4.dat to setProtoDuneSpacePoints.dat in that directory when and if you switch to the v4 geometry.

I close this report as the test problem is resolved. I do recommend opening a report for switching geometry when and if that is done.

#20 Updated by Dorota Stefan about 3 years ago

Hi David,

I am sorry, there was the discussion offline. We will use geo v4, Martin will make an update to the version v4 to resolve problems with the overlaps and make a simpler cathode. Martin will push changes by tomorrow so we use v4 geo for dunetpc v06_60_00.

Thanks,
Dorota

#21 Updated by Martin Tzanov about 3 years ago

Hi David,

I pushed a modified version of the protoDUNE v4 geometry. I believe
the failure was due to a bug in the global coordinate system origin spot.
The origin was shifted by about 4.5 cm which is about 9-10 in wire spacing coord.
Now the geometry should test OK with the original set of spacepoints (I hope).

Best,

Martin

#22 Updated by Dorota Stefan about 3 years ago

Martin, thank you so much.

#23 Updated by Christoph Alt about 3 years ago

Hi,

I can confirm that the new geometry (v4) now passes the test.

However, the new geometry caused some changes in the data product sizes of the g4 stage (see below).

If these changes are expected I will produce new reference files for protoDUNE SP.

Best,
Christoph


(lines with "<" show what is in the reference file,
lines with ">" show what is produced by the develop branch.)

v3:
1963: < G4 | largeant | | std::vector<simb::MCParticle> | 413
1964: < G4 | largeant | | std::vector<sim::AuxDetSimChannel> | 0
1965: < G4 | largeant | | art::Assns<simb::MCTruth,simb::MCParticle,void> | 413
1966: < G4 | largeant | | std::vector<sim::SimChannel> | 2309

v4:
1968: > G4 | largeant | | std::vector<simb::MCParticle> | 405
1969: > G4 | largeant | | std::vector<sim::AuxDetSimChannel> | 512
1970: > G4 | largeant | | art::Assns<simb::MCTruth,simb::MCParticle,void> | 405
1971: > G4 | largeant | | std::vector<sim::SimChannel> | 3047

full log: http://dbweb5.fnal.gov:8080/LarCI/app/ns:dune/storage/docs/2017/12/14/stdout_dtRbcpA.log

#24 Updated by Dorota Stefan about 3 years ago

Hi Christoph,

Thank you a lot. To me these changes are expected but let's wait for the confirmation from Martin.

Cheers,
Dorota

#25 Updated by David Adams about 3 years ago

Thanks for fixing the geometry and reporting it. It is good to know that the effort put into developing these geometry tests has paid off.

#26 Updated by Dorota Stefan about 3 years ago

It is correct that we have the size of AuxDetSimChannel different than zero because we have now more realistic implementation of CRT. Since the geometry changed it is also expected that the other products changed size, I think. If there are no objections in a couple of hours Christoph can make new reference files.

#27 Updated by Martin Tzanov about 3 years ago

Hi Christoph,

Yes, we do expect the size to change because now we have 64*8=512 CRT counters
which are implemented as AuxDetSensitive. I am not sure how this affects this
particular event (2GeV pion) in terms of MCParticle or SimChannel.

Best,

Martin

#28 Updated by Dorota Stefan about 3 years ago

I agree, it is difficult to judge this particular case, but since the geometry changed I would start worrying if we hadn't seen effect in the simulation.

#29 Updated by David Adams about 3 years ago

I have removed the version-specific test data files because Martin reports above that the v4 geometry should pass the v3 space point test.

#30 Updated by Christoph Alt about 3 years ago

I have updated the reference files for protoDUNE SP

Christoph

#31 Updated by Gianluca Petrillo about 3 years ago

Martin Tzanov wrote:

Yes, we do expect the size to change because now we have 64*8=512 CRT counters
which are implemented as AuxDetSensitive. I am not sure how this affects this
particular event (2GeV pion) in terms of MCParticle or SimChannel.

Just to be explicit:
  • it seems simb::MCParticle objects may be created within the counters, unless a volume filter is specified which excludes them (KeepParticlesInVolume configuration of LArG4); that would be enough to explain the change in number of particles
  • the addition or change of material in the detector (CRT counters) does change the path of the particle, just because of different energy loss
  • if the particle is generated inside the active volume (particle gun-like), the path in the TPC (which determines sim::SimChannels) is in principle not affected, since the counters are downstream...
  • yet, the moment the first particle (including secondaries) meets the new sensitive detector, simulation operations happen that did not happen before, triggering the extraction of more random numbers
  • since Geant4 uses a single random number stream, the additional simulation operations make the results diverge
So, in short:
  1. the difference in particle count is justified by the added sensitive volume
  2. change in signal is justified by physics via different energy deposition
  3. even when energy deposition is not affected, features of the simulation program randomness make the result different

Also available in: Atom PDF