[dataclasses] SuperDST should not produce pulses with the same time

[Jim Braun]

If the time rounding produces two pulses with the same time, they should be combined into one pulse with charge equal to the sum of the two original pulses.

Slack discussion:

lwille [4:21 PM]
Hey, I'm still running into an error while running Monopod. The error is FATAL (millipede): Assertion failed: p->GetWidth?() > 0 (MillipedeDOMCacheMap.cxx:379 in void MillipedeDOMCacheMap::UpdateData?(const I3TimeWindow&, const I3RecoPulseSeriesMap&, const I3TimeWindowSeriesMap&, double, double, double, bool)) This is happening with Nancy's NuGen_new datasets. It was suggested it might be an issue with @cweaver's new bunching method or maybe WaveDeform? (edited)
anyone else run into this issue or have any ideas of what's causing it/can fix it?

nwhitehorn [4:29 PM]
I don't see any way wavedeform can make a pulse with non-positive width
is this SuperDST data?

cweaver [4:32 PM]
This shouldn't be caused by the simulation compressions stuff, as that all happens well before digitization, so the worst it could do is make a weird shaped waveform.
I think the negative width can't be explicitly generated before wavedeform, although SuperDST is a reasonable suspect.
I've forgotten how we handle SuperDST in simulation, though; I think I repress the memories after each time I have to look into that section of processing.

dschultz [4:35 PM]
superdst should be almost exactly the same in simulation as in real data - it happens in L1 / pole filtering
however, I'm not sure what Nancy removed from the official script for her sample

cweaver [4:38 PM]
I doubt she would have tinkered with that.
(Also 'exactly the same' turns out to be horribly flexible when you're dealing with L1, dehydration, and rehydration. Pass2 was delayed many months because no one knows what that code is supposed to be doing.)

dschultz [4:40 PM]
yeah, at one time it was pretty close, then more exceptions kept being added
those scripts need a good cleaning again

cweaver [4:42 PM]
Anyway, to address lwille's problem, I think it would probably be most useful to isolate the data in question to determine whether the pulse really had a zero width when it came out of wavedeform, or whether something else later invented that. if the raw data for that frame exists or can be located so that the whole process can be run again step-by-step that could be very useful.

jimbraun [10:07 AM]
@lwille: I agree with Nathan that wavedeform cannot generate width <= 0. Also, if SuperDST was applied to the simulation, the problem is SuperDST or after. This is the first line in the SuperDST width compression:

i3_assert(width > 0 && width < double(std::numeric_limits<unsigned>::max()));

nwhitehorn [10:14 AM]
right -- I remember us being pretty paranoid about this

jimbraun [10:41 AM]
Here is the offending pulse series in one problematic event:
[I3RecoPulse:

Time : 10218

Charge : 11.075

Width : 1
Flags : LC ATWD FADC

], [I3RecoPulse:

Time : 10227

Charge : 29.175

Width : 0
Flags : LC ATWD FADC

], [I3RecoPulse:

Time : 10227

Charge : 17.475

Width : 1
Flags : LC ATWD FADC

],

jimbraun [11:11 AM]
Here is what I get directly from wavedeform for this event. I probably used a bad calibration for this, but I think this should still be enough for someone to figure out what’s wrong with the simulation SuperDST:
[I3RecoPulse:

Time : 10218.3

Charge : 11.3792

Width : 0.833585
Flags : LC ATWD FADC

], [I3RecoPulse:

Time : 10226.6

Charge : 28.1466

Width : 0.833585
Flags : LC ATWD FADC

], [I3RecoPulse:

Time : 10227.4

Charge : 18.5683

Width : 0.833585
Flags : LC ATWD FADC

]

lwille [12:21 PM]
@cweaver or anyone else would like to look at an example event with this error, here is one /data/ana/Cscd/StartingEvents/NuGen_new/NuTau/medium_energy/IC86_flasher_p1=0.3_p2=0.0_domeff_081/l2/1/l2_00000248.i3.zst Event 450

nwhitehorn [12:27 PM]
I think pretty cearly it is rounding the last two times to the same ns

cweaver [12:32 PM]
Milliepde is complaining about the width, though, and I didn't think the widths were related to the times of other pulses (doing so would not necessarily be consistent with their original definition in terms of basis function spacing, since one or more following basis functions might have been given zero amplitudes), and glancing at the SuperDST code seems to support this: ​http://code.icecube.wisc.edu/projects/icecube/browser/IceCube/projects/dataclasses/trunk/private/dataclasses/payload/I3SuperDST.cxx#L49

nwhitehorn [12:33 PM]
the rules for pulses are:
1) two pulses must not occur at the same time
2) pulses must have a non-zero positive width
3) no pulse may occur less than the preceding pulse's width after the time of an earlier pulse
(Note that #1 is a consequence of #2 and #3)
the second pulse here violates both rules 1 and 2

cweaver [12:35 PM]
Hm, fair point, so the identical times seem bad, even if they weren't what set off that specific assert.
(Indeed, had that assert not gone off, the one on the next line would have anyway.)

jimbraun [2:18 PM]
OK, the problem is in SuperDST
​http://code.icecube.wisc.edu/projects/icecube/browser/IceCube/projects/dataclasses/trunk/private/dataclasses/payload/I3SuperDST.cxx#L354
This is from Nov. 2011 (r81814), so it is not new behavior
I’m a bit surprised no one has reported this problem as far as I can remember, but it should be fixed

cweaver [2:23 PM]
That looks like it will only screw up the width once the times are bad?

jimbraun [2:23 PM]
Right, it is the time compression that messes up the times
and this then messes up the width
The correct behavior may be to just combine the pulses

cweaver [2:25 PM]
Unfortunately, it looks like the time compression is doing something fairly reasonable, given that it is expected to be lossy.
Yeah, I think that might be the best option.
Since if we chose to keep the compressed form it was meeting expectations for reproducing the original waveform.

jimbraun [2:26 PM]
Right

cweaver [2:26 PM]
It seems like to fix the immediate problem the merging should be added to the SuperDST decompression, although one would probably then want to add it during the compression as well (since it will save more space, and we're going to do it anyway).

jimbraun [2:28 PM]
Who maintains SuperDST?

dschultz [2:31 PM]
`# Maintainers
olivas@…
blaufuss@…
hightech-consulting@…
david.schultz@…`
I guess I do?!?

cweaver [2:31 PM]
Just one of those marvelous little surprises. :slightly_smiling_face:
I'm guessing the necessary code changes are simple, but the criticality of this makes testing a bit daunting.

[David Schultz]

Added a test in r164937/IceCube that fails for the pulses above. One goal is to make this test succeed.

[David Schultz]

Added merging on output, which passes the test. r164940/IceCube and r164941/IceCube.

This seems to break masks created from this superdst object, since there is one fewer pulse in the RecoPulseSeries? for a dom.

[David Schultz]

Changed from merging to 0.5 width pulses in r171894/IceCube.

[David Schultz]

Longer-term, we need a merge when superdst gets generated in the first place, or upstream if it comes from an I3RecoPulseSeries. That will eliminate the 0-width pulses at the source.

[Thomas Kintscher]

btw, I did report this before in #1712. Back then, Jim's fix resolved the issue.

As far as I can see, the fix is still in the current version of Millipede (in BinPulses around source:/IceCube/meta-projects/combo/trunk/millipede/private/millipede/MillipedeDOMCacheMap.cxx#L212).

However, the code has been reshuffled a little and now source:/IceCube/meta-projects/combo/trunk/millipede/private/millipede/MillipedeDOMCacheMap.cxx#L369 has the i3_assert(p->GetWidth() > 0), whereas a little bit further in L408 BinPulses gets called which correctly deals with pulses of width 0 (thanks to the fix above).
That means the i3_assert in L369 could be removed and the original issue that started the discussion on Slack would be settled (I verified this on my test event from above).

Whether SuperDST still needs a fix is then a different, but independent question (maybe depends on whether this is relevant elsewhere?).

[Alex Olivas]

Consolidated into one ticket: #2267