Fix for I3PulseNoiseTruthFlag extractor issue

[RasmusOrsoe]

Fixes #337 . Tested on upgrade data.

[asogaard]

Hi @RasmusOrsoe,

Thanks for proposing a fix! A few questions:

• I3PulseNoiseTruthFlagIceCubeUpgrade inherits from I3FeatureExtractorIceCube86, but wouldn't it make more sense for it to inherit from I3FeatureExtractorIceCubeUpgrade so as to avoid having to re-derive string, pmt_dir_x, etc.?
• If the purpose of I3PulseNoiseTruthFlagIceCubeUpgrade is to extract a truth label (truth_flag) would it make more sense to put it in i3truthextractor.py and remove the dependence on I3FeatureExtractor* entirely, thereby avoiding the cause of the problem you've encountered altogether?
  • If not, would it then be an option to use e.g. getattr(pulse, "charge", padding_value) instead of the include_pulses argument to fall back to a default of padding_value in case we get a pulse map that doesn't have the charge attribute?

[RasmusOrsoe]

Hi @RasmusOrsoe,

Thanks for proposing a fix! A few questions:

* `I3PulseNoiseTruthFlagIceCubeUpgrade` inherits from `I3FeatureExtractorIceCube86`, but wouldn't it make more sense for it to inherit from `I3FeatureExtractorIceCubeUpgrade` so as to avoid having to re-derive `string`, `pmt_dir_x`, etc.?

Yeah, I would think so. I did not change that part, but perhaps I should.

* If the purpose of `I3PulseNoiseTruthFlagIceCubeUpgrade` is to extract a truth label (`truth_flag`) would it make more sense to put it in `i3truthextractor.py` and remove the dependence on `I3FeatureExtractor*` entirely, thereby avoiding the cause of the problem you've encountered altogether?

Conceptually, I agree that this would be the best way to go. But technically, it's important to include DOM ids, PMT ids, perhaps even positions, in order to make sure that the flags are sorted correctly according to the pulsemap.

  * If not, would it then be an option to use e.g. `getattr(pulse, "charge", padding_value)` instead of the `include_pulses` argument to fall back to a default of `padding_value` in case we get a pulse map that doesn't have the `charge` attribute?

I suspect that would be a much more elegant solution. I did not think of that!

[RasmusOrsoe]

@asogaard this should be it, I think

[asogaard]

Fantastic!