Project

General

Profile

Bug #24328

Fix Geometry service feature reloading the geometry at begin of run

Added by Gianluca Petrillo 12 months ago. Updated 3 months ago.

Status:
Closed
Priority:
Normal
Category:
Geometry
Target version:
-
Start date:
04/20/2020
Due date:
% Done:

100%

Estimated time:
Occurs In:
Experiment:
-
Co-Assignees:
Duration:

Description

The Geometry service checks at each start of a run which geometry configuration the input data was generated with.
If that geometry is different from the currently configured one, it loads the one matching the input data.

This feature is buggy, broken and dangerous.

It is buggy, because it relies just on the name of the geometry to detect whether a new one is needed. Because it assumes the new geometry file is <GeometryName>.gdml, which is not an advertised requirement. Because it does assume that all the configuration of Geometry service except the detector name and source files are the same. Because it is not able to detect if a new channel mapping algorithm is required by the new geometry.

It is broken, because the requirement of flushing any cached geometry information at run boundaries is not advertised to downstream code.

It is dangerous, because the reload, if completed, may yield erroneous physics results.

This feature has been vocally deprecated. Given the reason above why the feature is broken, I consider it broken beyond repair.
My recommended fix is to leave the code check in geo::Geometry::preBeginRun(), with the understanding that if the test passes it does not guarantee the consistency of geometry (but if it fails it detects a certainly inconsistent one). As response to a failure of this check, the service should just throw a meaningful exception.
In alternative, this feature should be turned off by default and enabled only when explicitly requested by the user, and in that case it should emit information on the console about the feature being enabled, and about each time the geometry is reloaded (right now it does the latter only indirectly).

Associated revisions

Revision f952f595 (diff)
Added by Gianluca Petrillo 4 months ago

Configuration updates related to issue #24328

Revision 498378cd (diff)
Added by Gianluca Petrillo 4 months ago

Configuration updates related to issue #24328

Revision 625e18a1 (diff)
Added by Gianluca Petrillo 4 months ago

Configuration updates related to issue #24328

Revision 94cdd71f (diff)
Added by Gianluca Petrillo 4 months ago

Configuration updates related to issue #24328

Revision d84d75a8 (diff)
Added by Gianluca Petrillo 4 months ago

Configuration updates related to issue #24328

Revision 79262708 (diff)
Added by Gianluca Petrillo 4 months ago

Configuration updates related to issue #24328

Revision 73e6706c (diff)
Added by Gianluca Petrillo 4 months ago

Configuration updates related to issue #24328

Revision 09a2f492 (diff)
Added by Gianluca Petrillo 4 months ago

Configuration updates related to issue #24328

Revision de200c7f (diff)
Added by Gianluca Petrillo 4 months ago

Configuration updates related to issue #24328

Revision b098c23f (diff)
Added by Gianluca Petrillo 4 months ago

Configuration updates related to issue #24328

History

#1 Updated by Kyle Knoepfel 12 months ago

  • Status changed from New to Feedback

We'll contact you to learn more about the context of this request.

#2 Updated by Gianluca Petrillo 12 months ago

  • Assignee set to Gianluca Petrillo
  • Status changed from Feedback to Assigned

#3 Updated by Gianluca Petrillo 6 months ago

  • % Done changed from 0 to 80
  • Status changed from Assigned to Work in progress

The solution discussed with Kyle Knoepfel and Erica Snider is implemented in larcoreobj (pull request #9) and larcore (pull request #6).

Both the operative and technical details of the solution are documented in geo::Geometry class documentation.

In short:
  • GeometryConfigurationWriter, a new mandatory service, writes into each run the geometry configuration once;
  • Geometry service verifies that the geometry configuration of each new run is compatible with the current one.

In addition, provisions to convert existing sumdata::RunData information into something suitable for this check are also in place.

This starts the review phase of the work.

#4 Updated by Lynn Garren 3 months ago

The 10 PRs associated with this issue are now in larsoft v09_12_00.

#5 Updated by Kyle Knoepfel 3 months ago

  • % Done changed from 80 to 100
  • Status changed from Work in progress to Resolved

Gianluca, we have listed this issue as resolved. Feel free to close it as you like.

#6 Updated by Kyle Knoepfel 3 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF