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

Handle loading LST DL1 file from lstchain v0.9 #210

Merged
merged 8 commits into from
Apr 5, 2024

Conversation

gabemery
Copy link
Collaborator

No description provided.

@gabemery gabemery self-assigned this Feb 28, 2024
@Elisa-Visentin
Copy link
Collaborator

Fine for me. Here you are now assuming LST-1 ID = 1, so you loose a bit the IDs generalization, but we can accept this and fix it in the future by importing the configuration file (BTW, real data now only have ID =1)

@aleberti
Copy link
Collaborator

Well, it can be fixed now I would say. We can just simply pass the dictionary of the mc tel ids (not all the config, unless we need something else from there to be passed to the function), so that even if there are more LSTs added in the future in the real data, the function is already usable.

@Elisa-Visentin
Copy link
Collaborator

But I wanted to spare him some extra work both on the coincidence script and on the tests. He has already offered to fix the SubArray!

@gabemery
Copy link
Collaborator Author

@aleberti This fix is meant to handle loading dl1 data from existing lstchain v0.9 files. So it will never be applied to dl1 data from LST2-4.

In case a dl1 file contains the data from multiple telescopes (not sure if this is the plan) and such a fix is still needed in the future, the tel_ids could probably be read from the dl1 file.

Add tests with files created with lstchain v0.9.x
@aleberti
Copy link
Collaborator

Well, in any case the ID is changed later in the coincidence script. Also, we cannot even get the telescope ID from the broken subarray. So, not much to do anyway.

@aleberti aleberti added fix For fixes maintenance Maintenance related labels Mar 12, 2024
@aleberti
Copy link
Collaborator

There are no new comment since 2 weeks. @jsitarek green light from your side to merge? You have been assigned as 2nd reviewer.

magicctapipe/conftest.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jsitarek jsitarek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested one possible simplification, but since this is in the tests, it is also perfectly fine with me if you merge it as it is. Sorry for a big delay in checking this PR

@jsitarek
Copy link
Collaborator

jsitarek commented Apr 4, 2024

thx @Elisa-Visentin
as soon as the checks finishes correctly I think we can merge

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.06%. Comparing base (8663b3d) to head (dc91ab9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
+ Coverage   76.87%   77.06%   +0.18%     
==========================================
  Files          21       21              
  Lines        2560     2581      +21     
==========================================
+ Hits         1968     1989      +21     
  Misses        592      592              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jsitarek jsitarek merged commit 9dc7730 into master Apr 5, 2024
8 checks passed
@jsitarek jsitarek deleted the add_support_lstv09_files branch April 5, 2024 10:46
Elisa-Visentin pushed a commit that referenced this pull request Sep 12, 2024
Handle loading LST DL1 file from lstchain v0.9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix For fixes maintenance Maintenance related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants