This is meant to address the issue reported in issue #39

[SFBayLaser]

See #39

Generally, tbin is a short int in the range of a few "bins"... the offsets can be large (and event negative) and, if negative, will give nonsensical results for the tdc value. In comparing to the old SimChannel output module (DepoSetSimChannelSink) it seems clear that instead of simple "tbin" one really needs to substitite "m_tbins(tbin)/jm_tbins.binsize()".

Testing with ICARUS simlations in standard form give expected results with this change.

[FNALbuild]

A new Pull Request was created by @SFBayLaser (Tracy Usher) for develop.

It involves the following packages:

larwirecell

@LArSoft/level-1-managers, @LArSoft/level-2-managers can you please review it and eventually sign? Thanks.

cms-bot commands are listed here

[FNALbuild]

The code-checks are being triggered in jenkins.

[FNALbuild]

-code-checks
Pull request failed code-formatting checks. Please ensure that cetmodules has been setup and execute the following command from the top-level directory of your repository:

format-code \
  larwirecell/Components/DepoFluxWriter.cxx

Then commit the changes and push them to your PR branch.

[lgarren]

@SFBayLaser please run clang format as suggested

[SFBayLaser]

Wow! I am surprised there is a problem here... I modified one line of code (see comparison) and made no other changes so am really not understanding why it is failing clang-format.
Unfortunately, I seem unable to run clang-format on my local setup and I'm not finding instructions for resolving that. Is there a link where I can see what it is unhappy with?

[knoepfel]

@SFBayLaser, you need to setup cetmodules (it will be setup as part of your development environment). And then you should just type:

$ format-code <path to your larwirecell dir>/larwirecell/Components/DepoFluxWriter.cxx

[FNALbuild]

Pull request #40 was updated. @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please check and sign again.

[FNALbuild]

The code-checks are being triggered in jenkins.

[SFBayLaser]

Ok, I didn't realize we cared about fitting into the columns of an IBM card... should be good now.

[FNALbuild]

+code-checks

[knoepfel]

Thanks, @SFBayLaser. The clang-format file sets a width of 100 characters (a little wider than the IBM 80 limit), and it also places spaces around operators (like /). There's no perfect formatter, but it's better having consistent code than no automated formatting.

[knoepfel]

trigger build

[FNALbuild]

The tests are being triggered in jenkins.

[SFBayLaser]

As you say, no perfect format! Generally, I find the clang format hard to read but that is personal preference... and I'm not tasked with maintaining all of this! ;-)

[FNALbuild]

+LArSoft tests OK on slf7 for c7:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/20584&builds=
+build

[FNALbuild]

+LArSoft tests OK on slf7 for e20:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/20585&builds=
+build

[FNALbuild]

-icarus tests failed, with build warning,, with ignored warning for build, on slf7 for e20:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/ns:icarus/view_builds/index?offset=0&builds=icarus_ci/8154&builds=
for details of the parent CI build see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/20585&builds=

[FNALbuild]

-sbnd tests warning, with build warning,, with ignored warning for build, on slf7 for e20:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/ns:sbnd/view_builds/index?offset=0&builds=sbnd_ci/12674&builds=
for details of the parent CI build see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/20585&builds=

[FNALbuild]

-dune tests tests warning, with build warning,, with ignored warning for build, on slf7 for e20:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/ns:dune/view_builds/index?offset=0&builds=dune_ci/16080&builds=
for details of the parent CI build see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/20585&builds=

[FNALbuild]

-uboone tests warning on slf7 for e20:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/ns:uboone/view_builds/index?offset=0&builds=uboone_ci/9593&builds=
for details of the parent CI build see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/20585&builds=

[SFBayLaser]

ICARUS build failure unrelated to this PR. I believe there are updates in icarusalg for other things that collided here.

[lgarren]

@brettviren do you approve this PR?

[brettviren]

@SFBayLaser Hi Tracy.

I'm not following the goal of this change.

I would expect the user to set the reference_time configuration parameter to achieve any arbitrary time offset.

I think this PR will change the definition of what DepoFluxWriter claims to provide.

[SFBayLaser]

Hi @brettviren, the goal is to have the truth information contained in the SimChannels to match to, for example, hit information. In practice, the SimChannel information is in units of "TDC counts" while the hit information is returned in units of "ticks". Using ICARIUS as the example, the difference between the two is an offset - ticks are one unit on a waveform (for ICARUS that is 400 ns but I don't think this matters) with the first bin of the waveform tick = 0. TDC counts are the same in terms of bin size (400 ns) but the start is referenced to a "tdc clock start time). The document you provided describes how to include these offsets, I believe this has been done correctly in our json definitions:
`local wcls_simchannel_sink =
g.pnode({
type: 'wclsDepoFluxWriter',
name: 'postdrift',
data: {
anodes: [wc.tn(anode) for anode in tools.anodes],
field_response: wc.tn(tools.field),

  // time binning
  tick: params.daq.tick,
  window_start: -340 * wc.us, // TriggerOffsetTPC from detectorclocks_icarus.fcl
  window_duration: params.daq.readout_time,

  nsigma: 3.0,

  reference_time: -1500 * wc.us, // G4RefTime from detectorclocks_icarus.fcl

  smear_long: 0.0,
  smear_tran: 0.0,

  time_offsets: [std.extVar('time_offset_u') * wc.us, std.extVar('time_offset_v') * wc.us, std.extVar('time_offset_y') * wc.us],

  // input from art::Event
  sed_label: 'largeant:TPCActive',

  // output to art::Event
  simchan_label: 'simpleSC',
},

}, nin=1, nout=1, uses=tools.anodes+[tools.field]);
`
With these values the current release does not produce tdc values that make sense and there is no ability to match to hit information. In comparing DepoFluxWriter to the old DepoSetSimChannelSink it seemed clear that the issue is as described in issue #39. Certainly if I make the change to a local installation of larwirecell and then run I get "perfect" matching of SimChannel information to Hits (hmmm.... how do I include an image here?).

Ah! Example... so here the black curve is the raw waveform, blue an ROI of the deconvolved waveform, the purple is the SimChannel information from DepoFluxWriter and pink the SimChannel info from DepoSetSimChannelSink (in both cases the y have been scaled to fit the vertical scale)

I did not find any other way to include the offsets needed for ICARUS and produce correct results... happy to be shown that I am wrong...

[SFBayLaser]

I should add that there is some urgency to try to resolve this issue quickly... The ML effort on ICARUS needs this to be able to start working again, we have a collaboration meeting in 1 month and would like to be able to demonstrate the ML effort is ready for the start of Run 3 (which nominally starts the beginning of November). Thanks for helping to push this forward!

[brettviren]

Hi again, @SFBayLaser

I believe you want the tdc count to be relative to a time that includes the window_start time (in addition to the "G4RefTime").

This additional offset can simply be included in the definition of the reference_time configuration parameter. Since the reference_time is subtracted from the "nominal time" and the window_start is negative in your case, it would get added with a negative (hopefully I got that right). So, I think with the original C++ you can use this configuration line:

reference_time: -1500 * wc.us - self.window_start,

If this does not provide what you need, let's discuss more.

If it does give you what you need then I'm inclined to not merge this PR. The reason is that with this PR the user will implicitly get an unexpected arbitrary time offset (the window_start) from the nominal time. The documentation would need to be rewritten and users with different needs would need to actively undo this arbitrary time offset (with + self.window_start). Keeping the C++ as-is lets all the arbitrary time offsets be relative to a single nominal time and to be specified in one place (the config).

[SFBayLaser]

Ok, will try and report back. It looks like it will do what I need at the expense of using the window_start time in two places in the json file. I might suggest the documentation be updated to make that point as a user like me might not have thought of it.
I should also point out that our timing scheme was taken directly from MicroBooNE and is also the same for SBND... I would be surprised if not the same at ProtoDUNE. So I don't think this is necessarily "arbitrary" but more common practice. Whether that is true in the future I can't say...

[SFBayLaser]

I can confirm that Brett's proposal will work for us. Here is same waveform pic as before:

(note there are subtle differences in waveforms because I reran the detector simulation and it appears we don't start with the same seeds for the noise modeling)
So we can close both this PR and the corresponding issue.
Thanks!

[lgarren]

Thanks @SFBayLaser and @brettviren !

[brettviren]

@SFBayLaser Thanks so much for the thorough checks.

About the lack of reproducibility: this has a known cause which can actually be corrected in configuration. But to do that in a user-friendly way needs a little C++ work.

For reference, details are in this issue.

WireCell/wire-cell-toolkit#242