Bug #20685

Bug in ISCalcSeparate::EFieldAtStep

Added by Tingjun Yang over 2 years ago. Updated over 2 years ago.

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


Estimated time:
Occurs In:


Ajib Paudel found a bug that affects the calculation of the electric field in the simulation when SCE is enabled:

  212     if (fSCE->EnableSimEfieldSCE())
  213       {
  214         fEfieldOffsets = fSCE->GetEfieldOffsets(geo::Point_t{x,y,z});
  215         EField = std::sqrt( (efield + efield*fEfieldOffsets.X())*(efield + efield*fEfieldOffsets.X()) +
  216                             (efield*fEfieldOffsets.Y()+efield*fEfieldOffsets.Y()) +
  217                             (efield*fEfieldOffsets.Z()+efield*fEfieldOffsets.Z()) );
  218       }

The last two terms should be:
(efield*fEfieldOffsets.Y()*efield*fEfieldOffsets.Y()) +

I think this bug affected MicroBooNE MCC8 and ProtoDUNE MCC10 and should be fixed as soon as possible.

Lynn, please make sure this bug fix is included in this week's release as we are still preparing ProtoDUNE MCC11. Thanks.


#1 Updated by Kyle Knoepfel over 2 years ago

  • Status changed from New to Assigned

Mike, can this be ready for this week's release?

#2 Updated by Kyle Knoepfel over 2 years ago

We encourage you to use a library specially designed for these linear-algebra problems (e.g. SMatrix).

#3 Updated by Tingjun Yang over 2 years ago

Mike just pointed out this is the refactored larsim code, which was not used by MicroBooNE MCC8 or ProtoDUNE MCC10.

Apologies for the false alarm.

We still need to fix this for ProtoDUNE MCC11.

#4 Updated by Gianluca Petrillo over 2 years ago

Something like:

EField = efield * (geo::Xaxis() + fEfieldOffsets).R();

should do, except that it assumes electric field along X axis.
To know whether that is correct, access to TPC geometry is required (geo::TPCGeo::DriftDir()), but the service provider interface is not optimised for that.

Also it is necessary to pay attention to the relative sign of the offset and of the drift direction, and the absolute sign of the correction as used upstream. Just saying.

#5 Updated by Tingjun Yang over 2 years ago

  • Assignee changed from Michael Mooney to Wesley Ketchum

#6 Updated by Michael Mooney over 2 years ago

Note that this does not impact MicroBooNE MCC8 or ProtoDUNE MCC10, as Tingjun mentioned, but also shouldn't impact ProtoDUNE MCC11, as it does not yet make use of the refactored larsim code.

The original implementation of this feature (authored by me) can be found here:

The bug pointed out by Tingjun was introduced by Wes Ketchum, who has now been assigned the ticket. I think he has already created a feature branch with the fix.

#7 Updated by Lynn Garren over 2 years ago

Wes, can the change simply be merged with develop?


#8 Updated by Lynn Garren over 2 years ago

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

Since the feature branch was merged into larsoft v07_03_00 last week, I tested this change against the head of develop. The unit tests are fine except for ubana, which is broken for other reasons. This change is now in the head of develop and will be included in this weeks' larsoft release.

Also available in: Atom PDF