Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataWriter should not write pointing info from each event #1562

Closed
maxnoe opened this issue Dec 4, 2020 · 14 comments
Closed

DataWriter should not write pointing info from each event #1562

maxnoe opened this issue Dec 4, 2020 · 14 comments

Comments

@maxnoe
Copy link
Member

maxnoe commented Dec 4, 2020

The DataWriter should not try to reconstruct the monitoring info by looking into unique pointings.

Rather, it should have a write_pointing method, that gets the info that should be written.

This requires a common way to store the monitoring info however.

@maxnoe
Copy link
Member Author

maxnoe commented Dec 4, 2020

A simple solution would be to implement a PointingSource along the lines of EventSource, however for simtel files and standard DLx files, this would mean having to repeat the input file (once for EventSource, once for PointingSource, which is not nice UX.

event_source = EventSource(input_url)
pointing_source = PointingSource(input_url)
writer = DL1Writer(output_file)
for event in source:
    pointing_source(event)
    proces(event)

for pointing in pointing_source:
    writer.write_pointing(pointing)

@kosack
Copy link
Contributor

kosack commented Dec 4, 2020

yeah, this was discussed in a few past Issues as well.

see #1041 and the diagram here: #1165 (comment)

@maxnoe
Copy link
Member Author

maxnoe commented Dec 4, 2020

I opened another one because this is partciluar about pointing and the "hack" we currently do in the DataWriter.

For LST, this has the result of storing pointing info for every event although it would be sufficient to store the drive report values with a resolution of 2s.

@kosack
Copy link
Contributor

kosack commented Dec 4, 2020

But right now, the DL1 writer does not write per-event pointing, I think, so it should be ok.

Yes, the idea was to write drive reports ever few seconds, or even the average of them for real data.

@maxnoe
Copy link
Member Author

maxnoe commented Dec 4, 2020

But right now, the DL1 writer does not write per-event pointing, I think, so it should be ok.

Because for LST real data the pointing (altaz, tracking ra/dec) is different for each event, it is written for every event.

@kosack
Copy link
Contributor

kosack commented Dec 4, 2020

It depends a bit on the design: if we have event-wise (or at least rapidly updated pointing info), we could create a general MonitoringComponent that takes input eventwise containers, and writes out every N events a summary, with columns of value_mean, value_std, value_count to some output stream.

You could add that as a sub-component of DL1Writer, e.g. DL1Writer.pointing_monitor, and it would do the right thing.

That would be useful for other things we might want to reduce from Event → Monitoring stream.

Basically having two components: a MonitoringWriter and MonitoringInterpolator would be a nice overall design:

  • MonitoringWriter converts Event info into Monitoring tables
  • MonitoringInterpolator converts Monitoring tables back into Event info in memory.

@maxnoe
Copy link
Member Author

maxnoe commented Dec 4, 2020

For the lst data, that wouldn't make too much sense since the values come from the drive report with 2 s resolution, so it would make much more sense to just store these data values.

See: https://github.com/cta-observatory/ctapipe_io_lst/blob/ctapipe_compatible/ctapipe_io_lst/pointing.py

@kosack
Copy link
Contributor

kosack commented Dec 4, 2020

For the lst data, that wouldn't make too much sense since the values come from the drive report with 2 s resolution, so it would make much more sense to just store these data values.

Yes, even easier then, and no special code needed. So the question is how to write something at a different rate than the Event stream, when the writing happens in an event loop.

Options:

  • don't write it during event processing, but rather attach the full pointing monitoring info to the file afterward (or even to a separate file, which could be linked to the first using HDF5 data set links)

  • add a mechanism to detect when something changes in the pointing, and only call TableWriter.write() if that happens

I guess this is only a problem if you want to write DL1 data from LST1, which won't be a problem when DL0 is defined. DL1Writer would then do what it does now, and just copy the pointing from DL0.

@kosack
Copy link
Contributor

kosack commented Dec 4, 2020

I'm a bit confused though: you said:

For the lst data, that wouldn't make too much sense since the values come from the drive report with 2 s resolution, so it would make much more sense to just store these data values.

but also:

Because for LST real data the pointing (altaz, tracking ra/dec) is different for each event, it is written for every event.

Which is it? I would separate pointing corrections from drive reports as well.

I'd guess you only need:

  • drive pointing every 2 seconds or so
  • pointing corrections at a simliar rate (not necessarily the same though)

From which you can generate per-event pointing on the fly.

Also, a question: what data are include in pointing data?

  • ALT/Az as measured by the drive encoders
  • also velocities? (makes interpolation easier, and can deal with less frequent monitoring values)

@maxnoe
Copy link
Member Author

maxnoe commented Dec 4, 2020

Which is it? I would separate pointing corrections from drive reports as well.

Both actually. It writes the current measured zen/az and the nominal (not measured) ra/dec.

@kosack
Copy link
Contributor

kosack commented Dec 4, 2020

I think all of this comes down to mixing a few things.

  1. it is not the responsibility of the DL1Writer to write hardware level pointing information, that should be written to the DL0 files (can be separate from event data, in another file for example), as well as pointing correction information
  2. one of the calibrations done in the DL0-DL1 step should be the application of pointing corrections, and output of "DL1 pointing" information, which is not necessarily the same as the DL0 pointing information generated by the telescope (it can be simplified).
  3. we then need to be able to go from DL1/Monitoring/Pointing information to DL1/Event/Pointing information, meaning some notion of interpolation of the pointing info back to event time, which has to be more than just a corrected Alt/Az position, it should also include camera rotation and related corrections (focal length, maybe skew if needed for bendy structures).

So the problem is not just writing back out what the drive reports give, that is DL0 - here we need a step that goes from DL0 → DL1, and puts the pointing in a common place. No RA/Dec is needed, only Alt/Az, by the way.

So I would imagine we need containers for DL1 Event-wise and Monitored pointing info, and they should have at least fields of (alt, az ) with pointing shifts applied already, and rotation_correction, focal_length_correction, which are needed to properly calculate Hillas parameters and to transform to sky coords.

If you are implemeting an event source that goes from R0 → DL1 completely, you would need some conversion of the internal LST pointing into to this format, and we need a general tool to interpolate to event time.

@kosack
Copy link
Contributor

kosack commented Dec 4, 2020

Anyhow, I think this requires a meeting and discussion offline, since it is a general and larger problem and already has several issues related to it.

Adding a DL1Writer.write_pointing() method is however fine as a first step to separating writing of event-wise and monitoring. The question is (from the #1041 as well) where to put non-event-wise mounting info, since it should not really be in the "event" structure.

@maxnoe
Copy link
Member Author

maxnoe commented Dec 4, 2020

it is not the responsibility of the DL1Writer to write hardware level pointing information, that should be written to the DL0 files (can be separate from event data, in another file for example), as well as pointing correction information

Exactly and this is why I opened this issue. We should implement another way to write the pointing info for simtel files so it does not mess with this.

@maxnoe maxnoe mentioned this issue Jul 30, 2021
2 tasks
@maxnoe maxnoe changed the title DL1Writer should not write pointing info from each event DataWriter should not write pointing info from each event Jun 20, 2023
@maxnoe
Copy link
Member Author

maxnoe commented Feb 27, 2024

This was fixed in #2438

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants