Project

General

Profile

Bug #24600

OpFastScintillation only propagates light in the first cryostats

Added by Gianluca Petrillo about 1 month ago. Updated 10 days ago.

Status:
Closed
Priority:
Urgent
Category:
Simulation
Target version:
-
Start date:
07/06/2020
Due date:
% Done:

100%

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

Description

It appears that OpFastScintillation, in charge of simulating and propagating scintillation light to the optical detectors, does so only for part of the first cryostat.
This bug was introduced with commit larsim:commit:993846e, which relies on an experiment-dependent selection of coordinates which was in the code since earlier but was not used except for diagnostic messages.

There are two issues with this implementation:

1. the many, very essential experiments with more than one cryostat (a.k.a. ICARUS) do not see any light simulated in cryostats beyond C:0
2. all the experiments do not see any scintillation light originating from outside the volTPC volume; there is scintillating liquid argon outside that volume, and lookup table photon visibility libraries do support the extra volume

There is clearly some intention behind the introduction of this change, and this should be clarified in order to find the proper solution.
The author of the change, Iker de Icaza Astiz, is added to the watcher lists.


Incidentally, the best way to get a volume including all the TPC in the detector is something like:

geo::BoundedBoxGeo allTPCs;
for (geo::TPCGeo const& tpc: geo->IterateTPCs()) allTPCs.ExtendToInclude(tpc);

and the check of a point being contained is
return allTPCs.ContainsPosition(point);
for a point of type geo::Point_t and
return allTPCs.ContainsPosition(point.data());
for a point of type std::array<double, 3U>.
To get one boundary per cryostat:
std::vector<geo::BoundedBoxGeo> allTPCs;
for (geo::CryostatGeo const& cryo: geo->IterateCryostats()) {
  geo::BoundedBoxGeo box;
  for (geo::TPCGeo const& tpc: cryo.IterateTPCs()) box.ExtendToInclude(tpc);
  allTPCs.push_back(std::move(box));
} // for cryostat

Sprinkle with auto wherever you think the code is too clear.


Related issues

Related to LArSoft - Necessary Maintenance #24601: Scintillation visibility by semi-analytic parametrization not supported for multi-cryostat detectorsAccepted07/06/2020

History

#1 Updated by Gianluca Petrillo about 1 month ago

  • Subject changed from OpFastScintillation does not propagate light in cryostats beyond the first to OpFastScintillation only propagates light in the first cryostats

#2 Updated by Gianluca Petrillo about 1 month ago

  • Related to Necessary Maintenance #24601: Scintillation visibility by semi-analytic parametrization not supported for multi-cryostat detectors added

#3 Updated by Lynn Garren about 1 month ago

The commit mentioned was introduced in larsoft v08_52_00 (larsim v08_24_00) as part of larsim PR 14.

#4 Updated by Iker de Icaza Astiz 29 days ago

I dissent from the statement that this is a bug. The code never had the capability of detecting light beyond the current tpc. In the mentioned commit the only thing I did was to encapsulate the test that already existed and made evident the limitation of the current implementation.

It's been acknowledged that moving forward to enable this new feature will require a number of changes. For instance using a method similar to the way it currently accounts the passage of photons through the wires to compute the passage of photons through the semi transparent cathode that ICARUS has.

Currently there's two functions that test wether the light coming from a ScintPoint has the conditions suited to the approach: isOpDetInSameTPC() and isScintInActiveVolume(), maybe these two could be fused into a more comprehensive canPhotonsReachOpDet()

#5 Updated by Gianluca Petrillo 29 days ago

Iker de Icaza Astiz wrote:

I dissent from the statement that this is a bug. The code never had the capability of detecting light beyond the current tpc. In the mentioned commit the only thing I did was to encapsulate the test that already existed and made evident the limitation of the current implementation.

To the best of my knowledge, this is not a limitation for the visibility via lookup table method ("photon library"), and the check being questioned is in a path shared by the different methods.

I also assume that your comment is only about the second point. If you consider not a bug for light being propagated only if in the first cryostat1, I think you will have to explain why.

(I might be reading the code wrong though?)


1 Edit Clarification: light scintillating in the other cryostats is not propagated.

#6 Updated by Gianluca Petrillo 29 days ago

  • Description updated (diff)

#7 Updated by Iker de Icaza Astiz 28 days ago

Ok sorry, I miss the fact that you're referring to the lookup table method. I assumed you were using the semi-analytic approach to generate light in ICARUS.

I'll take a look, should be easy to fix. I'll get in touch directly with you for further clarification.

#8 Updated by Gianluca Petrillo 28 days ago

We agree that the "solution" is to limit the current behaviour to when the semi-analytic model is used only.
In addition, Iker pointed to some code locations that I had missed, which work only under the assumption that the cathode is completely opaque.
In those locations, the assumption is removed unless it is code specific to the semi-analytic model. I am not sure if this is a problem for ProtoDUNE, as I don't remember if the cathode has any transparence.

In summary, the intended patched behaviour is:
  • if semi-analytic model is not used, scintillation from any point p in the cryostat sensitive volume is propagated to all PMT
  • if semi-analytic model is used, scintillation from a point p is propagated only if p is in the active volume of a TPC (i.e. within the field cage) and only toward the PMT facing that same TPC (not even to any PMT on the opposite side with respect to the cathode)

The code will not be supporting generic detectors until additional issues are solved.

Technical discussion on the implementation of this guideline is moved to larsim pull request.
That code has been verified to produce light output throughout the full ICARUS detector.

#9 Updated by Kyle Knoepfel 24 days ago

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

We will review the PR.

#10 Updated by Gianluca Petrillo 24 days ago

  • Priority changed from Normal to Urgent

Thank you.
I should have pointed out that this issue is critical for ICARUS, with the photon simulation being corrupted and all.

#11 Updated by Alexander Himmel 23 days ago

This should be a non-issue for protoDUNE on two fronts.
1) The cathode is opaque.
2) We are using libraries, not parameterizations, in protoDUNE.

#12 Updated by Gianluca Petrillo 23 days ago

In my reading of the code, the limitation of the volume to just the TPC active volumes prevents scintillation light from behind the anodes and beyond the field cage to be propagated as well.
If that reading is correct, ProtoDUNE might be affected.

#13 Updated by Kyle Knoepfel 17 days ago

  • % Done changed from 0 to 100
  • Status changed from Assigned to Resolved

PR 30 addressing this issue was included in last week's release.

#14 Updated by Kyle Knoepfel 10 days ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF