Add DL3 writer#2979
Conversation
|
kosack
left a comment
kosack
left a comment
There was a problem hiding this comment.
I'm still working through this, so this is not yet a complete review, but here are a few initial comments
| return np.rad2deg(result_rad) % 360 | ||
|
|
||
| @staticmethod | ||
| def _circular_mean(angles_deg): |
There was a problem hiding this comment.
I'd suggest just using quantities here:
| def _circular_mean(angles_deg): | |
| def _circular_mean(angles: u.Quantity[deg]) → u.Quantity[deg]: |
And that simplifies the code, since numpy will use the correct angle unit automatically:
In [1]: a = [0,45,90] *u.deg
In [2]: np.sin(a)
Out[2]: <Quantity [0. , 0.70710678, 1. ]>
In [3]: np.sin(a.to_value("rad"))
Out[3]: array([0. , 0.70710678, 1. ])
Then below, you can drop using np.rad2deg, and it should all work.
There was a problem hiding this comment.
Or better yet, why not just use astropy's implementation
|
|
||
| @pytest.fixture(scope="session") | ||
| def dl2_events_for_dl3(single_obs_gamma_diffuse_full_reco_file, dl2_meta_for_dl3): | ||
| preprocessor = DL2EventPreprocessor( |
There was a problem hiding this comment.
If you like, I can open a PR to this branch to convert this to use the new EventPreprocessor, which could simplify a lot of the code.
There was a problem hiding this comment.
See #3004 where I started to see what changes I need to EventPreprocessor to support the DL2 to DL3 transition
| __all__ = ["DL3EventsWriter", "DL3GADFEventsWriter"] | ||
|
|
||
|
|
||
| class DL3EventsWriter(Component): |
There was a problem hiding this comment.
My main comment here is that I think that the DL3 writer should not store state of the observation.
The inputs to the actual writing function should be the event list, the irfs and additionally needed data, but it should not keep that as state on the writer instance.
It should be possible to write multiple observation blocks using the same instance of the writer.
|
|
||
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) | ||
| self._obs_id = None |
There was a problem hiding this comment.
It looks more like this is a data class storing all information that should go into a DL3 file, not a writer.
3 participants
Add a DL3 writer for the GADF format to catapipe.
This PR is sub element of #2727 to simplify the review.