Bug #14964

OpFlashAlg incompatible with multi-TPC detectors

Added by Gianluca Petrillo over 4 years ago. Updated over 3 years ago.

Target version:
Start date:
Due date:
% Done:


Estimated time:
Spent time:
Occurs In:


At a first glance to the code, opdet::GetHitGeometryInfo() ( larana:source:larana/OpticalDetector/OpFlashAlg.cxx line 501) uses the variant of geo::GeometryCore::NearestWire() which does not take cryostat and TPC number:

double PEThisHit = currentHit.PE();
for (size_t p = 0; p != geom.Nplanes(); ++p) {
  unsigned int w = geom.NearestWire(xyz, p);  += PEThisHit*w; += PEThisHit*w*w;

This yield to the nearest wire in TPC 0, which would very often result in a geo::InvalidWireError being thrown. DUNE channel mapping on the other end has a special NearestWireID() which does not throw an exception (and returns a "capped" wire number). Either way, it's a problem.

This analysis is based on reading code (plus observing that exception being thrown when using a more standard NearestWireID() in DUNE 35t test), and I would expect it to yield an excess of flashes with wire centers on these capped values.
This needs to be confirmed with the experiment.

Associated revisions

Revision 5fa398ea (diff)
Added by Gianluca Petrillo about 4 years ago

OpFlashAlg now looks for the right TPC.

This solves issue #14964 .

Revision 16fd1c70 (diff)
Added by Gianluca Petrillo about 4 years ago

OpFlashAlg now looks for the right TPC.

This solves issue #14964 .


#1 Updated by Alexander Himmel over 4 years ago

OK, I've done some more digging. The last person to touch this bit of code was Gleb, but all he did was fix up the indentation. But this code dates back to Wes Ketchum in 2014 when this code was first implemented.

As for the effect, it seems to only influence the sumw and sumw2 variables, and those only influence the WireCenters and WireWidths variables, which I have to admit I did not know (or forgot) existed until I started looking into this incident. So, in practice, I think DUNE never noticed or cared about this issue before.

Is there a simple way to fix this, Gianluca? We should be able to get the TPC or any other information from the geom.OpDetGeoFromOpChannel(currentHit.OpChannel()) call, which currently is only used to get the position of the photon detector in space.

#2 Updated by Gianluca Petrillo over 4 years ago

Ah, that's another way.

I was thinking to provide a geometry function similar to NearestWireID which would detect on its own which TPC you are in first. That would still require to know which plane you care of, of course. But your suggestion may be more pertinent.
I have to check whether you can discover the TPC from the optical detector. In fact, does the question which TPC an optical detector belongs to even make sense in DUNE far detector?

#3 Updated by Alexander Himmel over 4 years ago

That's a good point. There's clearly some ambiguity here since most photon detectors will sit in between two drift regions and correspond to wires on either side. This may be more complicated than anticipated. We might want to talk to uBooNE experts about what the intended behavior here is so we can figure out a multi-tpc analogue that makes sense.

#4 Updated by Gianluca Petrillo about 4 years ago

  • Status changed from New to Assigned
  • Assignee set to Gianluca Petrillo
  • % Done changed from 0 to 80

I have pushed a fix in branch feature/gp_Issue14964 of larana (commit larana:5fa398ea53fddf3de3f3313fd249c14292cf00b0).
My choice was to ignore all the light from outside the TPC volume, only for the purpose of getting the expected hit position in wire coordinates.
The implementation of NearestWire() is such that it can't be trusted for points outside the TPC, and it should not be used in that case.
What it does: it assumes the wires have infinite length. And it gives the nearest wire accordingly. Whether it reports an error if no infinite wire is close to the point, it depends on the implementation, which is in ChannelMapAlg and therefore experiment-specific. I remember DUNE having a behaviour different than the "standard" one in this last respect.
I suspect that the difference is small, and I believe the new implementation is, if anything, more correct than the old one. But I would appreciate your testing before I merge it into develop.

#5 Updated by Alexander Himmel about 4 years ago

Like I said Gianluca, we don't use these wire variables, so I don't think we have any easy way to test this and see if it is behaving sensibly.

#6 Updated by Katherine Lato about 4 years ago

Need to confirm that the patch doesn't break anything.
Asking Wes Ketchum.

#7 Updated by Katherine Lato over 3 years ago

  • Assignee changed from Gianluca Petrillo to Alexander Himmel

Assigned to Alex with his permission. (Although it will be a bit since he's on paternity leave.)

#8 Updated by Alexander Himmel over 3 years ago

  • Assignee changed from Alexander Himmel to Katherine Lato

Just started back at work, and took a look at this issue. It makes no sense to "assign" this to me. Gianluca has already implemented a fix almost a year ago, and the only thing to do is to have someone from MicroBooNE check to make sure it doesn't break anything. We don't have anything in DUNE which uses this information, so we have no meaningful way to test this. Once it's been fixed, we can maybe explore using it on a longish timescale.

Katherine, I'm re-assigning this back to you, and I suggest you find a MicroBooNE PD expert to take this on.

#9 Updated by Katherine Lato over 3 years ago

  • Assignee changed from Katherine Lato to Wesley Ketchum

Moving forward with the understanding that DUNE is not affected by the change and will independently do the necessary testing when and if they decide to use this algorithm.
Wes, care to take a look at this?

Also available in: Atom PDF