Ctapipe 25 #101
Ctapipe 25 #101
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #101 +/- ##
==========================================
- Coverage 90.57% 90.50% -0.07%
==========================================
Files 11 11
Lines 1337 1338 +1
==========================================
Hits 1211 1211
- Misses 126 127 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I had a look as well, and nothing suspicious caught my attention. We need to make sure to make a new version when this is merged (maybe bump to 0.6 ?) |
ctapipe_io_magic/__init__.py
Outdated
| @@ -885,6 +886,7 @@ def prepare_subarray_info(self): | |||
|
|
|||
| # MAGIC telescope positions in m wrt. to the center of MAGIC simulations, from | |||
| # CORSIKA and reflector input card and recomputed (rotated) to be w.r.t. geographical North | |||
| reference_location = EarthLocation.of_site('lapalma') | |||
There was a problem hiding this comment.
I am wondering if we should use the same default as the one used in ctapipe_io_lst, i.e. in https://github.com/cta-observatory/ctapipe_io_lst/blob/main/src/ctapipe_io_lst/constants.py#L78 . @jsitarek any opinion? I think it does not matter much, but better to have a "good" default already now and change it only if really needed later on.
There was a problem hiding this comment.
indeed it would be safer, especially that the height is also one of the paramters
There was a problem hiding this comment.
fixed! Thank you :)
| @@ -17,6 +17,7 @@ | |||
| from astropy.time import Time | |||
| from pkg_resources import resource_filename | |||
|
|
|||
| from ctapipe_io_lst.constants import REFERENCE_LOCATION | |||
There was a problem hiding this comment.
mmmh I would not have introduced a dependency also on ctapipe_io_lst ... I would assume that those coordinates will not change frequently (if at all, maybe when more than one LST is included, but that's quite in the future). I would probably go into the direction of including those constants in constants.py and update them if really needed. @jsitarek opinions?
There was a problem hiding this comment.
I do not have a strong opinion here. For ctapipe_io_magic this is an extra dependency which obviously is best to avoid if needed, but ctapipe_io_magic is either way always working together with magic-cta-pipe, where we have lstchain dependency, that needs ctapipe_io_lst, and all the versions still need to agree, so I do not see a big issue to leave it as it is now. But the option that you proposed is also fine with me.
There was a problem hiding this comment.
Maybe we can discuss that on Wednesday, so that we can collect opinions also from the rest of the group? Both options are fine for me, we just have to choose, but this is not extremely urgent
No description provided.