icecube / skywriter

Upstream Tools for SkyDriver & the Skymap Scanner
MIT License
0 stars 0 forks source link

Recover keys from original P-frames in `i3_to_json.py` #7

Closed mlincett closed 11 months ago

mlincett commented 11 months ago

Previously i3_to_json.py would fill all the keys required by the realtime JSON generation code with dummy values. This was likely an unintended consequence of running the trigger splitter to create SplitUncleanedInIcePulses, which would result the creation of an empty P-frame. See #5 and #6 for context.

With this PR this behavior changes:

To achieve this, some machinery is introduced for this in the form of the fill_key() and fill_missing_keys() functions. The latter replaces functionality previously implemented in alertify().

Additional changes:

mlincett commented 11 months ago

Thanks for this. In lieu of running a separate tray, what about adding an optional argument -p for a user-specified pulse-series, which can be copied into SplitUncleanedInIcePulses? Then we can just run over the original p-frame.

This would mitigate some unintended behavior when there is a clash of uid.

It's a possibility, but how do we establish that the user-specified pulse-series is the correct one / equivalent to SplitUncleanedInIcePulses ? For example I would not know which one to pick and I would rather encode default and fallback options rather than leaving it open.

Then I would still go for an opt-in method to choose which keys to copy and avoid flooding the output file with unnecessary data (also it seems the content of at least a couple of keys is not serializable).

tianluyuan commented 11 months ago

Thanks for this. In lieu of running a separate tray, what about adding an optional argument -p for a user-specified pulse-series, which can be copied into SplitUncleanedInIcePulses? Then we can just run over the original p-frame. This would mitigate some unintended behavior when there is a clash of uid.

It's a possibility, but how do we establish that the user-specified pulse-series is the correct one / equivalent to SplitUncleanedInIcePulses ? For example I would not know which one to pick and I would rather encode default and fallback options rather than leaving it open.

Then I would still go for an opt-in method to choose which keys to copy and avoid flooding the output file with unnecessary data (also it seems the content of at least a couple of keys is not serializable).

Yes that's true, it's not possible ensure the passed pulseseries will match SplitUncleanedInIcePulses. Though it would allow some additional flexibility if users wanted to run scans on different pulses.

I think the opt-in method would work (and already exists to some extent with the --extra). Maybe one way to get around the UID for MC is to hash the InIcePulses, which would be unique for all intents and purposes.

mlincett commented 11 months ago

It's a possibility, but how do we establish that the user-specified pulse-series is the correct one / equivalent to SplitUncleanedInIcePulses ? For example I would not know which one to pick and I would rather encode default and fallback options rather than leaving it open. Then I would still go for an opt-in method to choose which keys to copy and avoid flooding the output file with unnecessary data (also it seems the content of at least a couple of keys is not serializable).

Yes that's true, it's not possible ensure the passed pulseseries will match SplitUncleanedInIcePulses. Though it would allow some additional flexibility if users wanted to run scans on different pulses.

This should be rather a feature for Skymap Scanner :)

I think the opt-in method would work (and already exists to some extent with the --extra). Maybe one way to get around the UID for MC is to hash the InIcePulses, which would be unique for all intents and purposes.

I guess this can be done but we still have the issue of the output filename (I would avoid long hashes in filename if possible).

ric-evans commented 11 months ago

That's a lot of diff. What does it look like if you run black with --skip-string-normalization?

FWIW this repo doesn't have much history anyway, so I think the big diff is okay