Enable access to subarray information from file before event loop #1157
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1157 +/- ##
==========================================
+ Coverage 85.93% 85.98% +0.04%
==========================================
Files 182 182
Lines 11365 11392 +27
==========================================
+ Hits 9767 9795 +28
+ Misses 1598 1597 -1
Continue to review full report at Codecov.
|
|
yes, this will be solved with the major restructuring of the eventsource/instrument that I proposed a while ago. I should really write up a more detailed description. Related are:
|
|
I think #1041 is the most relevant: see the proposals there for how to separate it. My preference would be to have it be an attribute that can be accessed from an EventSource (header info), or have it returned in the event source loop (less ideal). |
| @@ -190,6 +190,18 @@ def is_stream(self): | |||
| """ | |||
| return False | |||
|
|
|||
| @property | |||
| @abstractmethod | |||
| def subarray(self): | |||
There was a problem hiding this comment.
I don't know if that's a good idea. Are we we sure, subarray info can come from an event source?
I don't think this is the case for e.g. LST real data event source
There was a problem hiding this comment.
In the current scheme the event source must provide it in one way or another (to attach it to the event container), even if the information is not located in the file. So I don't think this change should be a problem.
There was a problem hiding this comment.
yeah, the name EventSource may be confusing, but that's the idea - it provides the events + any other data associated with them (it already does this in fact! Just mixes the two concepts, so I don't see a problem)
|
So the inst container is already deprecated. On second thoughts, I would like to avoid doing the same for the MCHeader in this PR. It isn't as simple to disentangle from the event building as the SubarrayDescription as its being updated per event at the moment... Can this PR be accepted? |
|
[moved comment to #1041 ] |
|
Ok, lets leave the big .changes for another PR. I'll move my previous comment to the issue |
| @@ -51,6 +51,12 @@ def is_compatible(file_path): | |||
| '''This class should never be chosen in event_source()''' | |||
| return False | |||
|
|
|||
| @property | |||
| def subarray(self): | |||
| with self.pyhessio.open_hessio(self.input_url) as file: | |||
There was a problem hiding this comment.
this of course will fail if the file is a stream. Do we want to break that idea? I think you can change the logic to still work with streams.
There was a problem hiding this comment.
Its done this way because of the limitation of the hessio library. Do we have to account for streams with hessio?
pyeventio allows the SubarrayDescription to be generated before starting the event loop.
There was a problem hiding this comment.
The HESSIOEventSource is no longer maintained (and is deprecated), so I think we can drop it. We can remove the cross-check test in the next version., and just remove hessioeventsource.py
* remotes/upstream/master: Move gain selection from CameraCalibrator to EventSource (cta-observatory#1167) Pointing container (cta-observatory#1141) Replace resolve with __getitem__ Correct variable name Add resolving to the TelescopeParamter fixed bug where telescope name and TelescopeDescription were conflused Add instructions for Slack # Conflicts: # ctapipe/io/tests/test_simteleventsource.py
In #1155 I encountered another situation where it would be useful to have the subarray information available immediately after initialising the eventsource (before starting the event loop).
This appears to be a fairly straightforward change, which could help keep a lot of other code tidy.
This idea was originally suggested in #722