Last updated scripts (4-LSTs): Torino group by Elisa-Visentin · Pull Request #180 · cta-observatory/magic-cta-pipe

Elisa-Visentin · 2023-12-05T16:06:41Z

Here the last updated analysis steps (RFs, DL2, DL3): new RF implementation (one per telescope instead of one per telescope per combination).
Again a huge PR, and it would be great if we could merge it before the LST school (to remove hardcoded variables and duplicated functions due to the new-old glueing at the DL1-stereo level).
The Doc strings must still be fixed a bit (to pass the tests), but you can already take a look into it.
Thanks

aleberti · 2023-12-05T16:27:16Z

Hi, many changes are in the documentation of the functions, but most of them, they should not have been changed. I.e. the spaces removed between the name of the parameter and the colon. I think you have some setting in your editor which is removing them automatically, but that makes the lint and docs checks fail.

codecov · 2023-12-05T16:34:22Z

Codecov Report

❌ Patch coverage is 92.39130% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.36%. Comparing base (9b8776e) to head (e5d8a1a).
⚠️ Report is 1069 commits behind head on master.

Files with missing lines	Patch %	Lines
magicctapipe/io/io.py	85.41%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
- Coverage   76.17%   75.36%   -0.82%     
==========================================
  Files          21       21              
  Lines        2493     2419      -74     
==========================================
- Hits         1899     1823      -76     
- Misses        594      596       +2

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Elisa-Visentin · 2023-12-05T16:59:11Z

Hi, we had to substitute some of the functions/whole scripts with the newer version, thus loosing some spaces in the copy paste (only in the Doc strings; I checked the scripts and no previous analysis update has been lost). But fixing them now is much easier than trying to integrate all the updates in the master to preserve the doc strings (we changed a lot of code lines, and on a fork created months ago, so even the automatic git merge fails and manual merging is a pain in the neck, as we discovered during the first PR). But I will fix them tomorrow: BTW, they do not interfere with the code tests and the scripts (need only to be fixed before the merge just not to 'propagate' do string issues to the next PRs) Il mar 5 dic 2023, 17:34 codecov[bot] ***@***.***> ha scritto:

…

Codecov <https://app.codecov.io/gh/cta-observatory/magic-cta-pipe/pull/180?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory> Report Attention: 4 lines in your changes are missing coverage. Please review. Comparison is base (5a2e33b) <https://app.codecov.io/gh/cta-observatory/magic-cta-pipe/commit/5a2e33bb70b2a4e5011d81d6adf533c08d4496b0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory> 76.13% compared to head (f2d39a9) <https://app.codecov.io/gh/cta-observatory/magic-cta-pipe/pull/180?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory> 75.32%. Files <https://app.codecov.io/gh/cta-observatory/magic-cta-pipe/pull/180?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory> Patch % Lines magicctapipe/io/io.py <https://app.codecov.io/gh/cta-observatory/magic-cta-pipe/pull/180?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-bWFnaWNjdGFwaXBlL2lvL2lvLnB5> 86.20% 4 Missing ⚠️ <https://app.codecov.io/gh/cta-observatory/magic-cta-pipe/pull/180?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory> Additional details and impacted files @@ Coverage Diff @@## master #180 +/- ## ==========================================- Coverage 76.13% 75.32% -0.82% ========================================== Files 21 21 Lines 2489 2407 -82 ==========================================- Hits 1895 1813 -82 Misses 594 594 ☔ View full report in Codecov by Sentry <https://app.codecov.io/gh/cta-observatory/magic-cta-pipe/pull/180?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory> . 📢 Have feedback on the report? Share it here <https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory> . — Reply to this email directly, view it on GitHub <#180 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A43O4NHD55ZY4ZAZOL2TY7DYH5EJVAVCNFSM6AAAAABAH6AENKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBRGE3DANJUG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- ------------------------ Indirizzo istituzionale di posta elettronica degli studenti e dei laureati dell'Università di TorinoOfficial University of Turin email address for students and graduates

Elisa-Visentin · 2023-12-06T07:24:48Z

Fixed :)

aleberti · 2023-12-08T15:33:44Z

I do not have any major comment. @jsitarek can you also have a look? it should be fast anyway.

jsitarek

I left a few comment/questions

jsitarek · 2023-12-13T07:24:26Z

        # The events of the MAGIC-stereo combination are excluded
-        df_events.query("(combo_type > 0) & (magic_stereo == True)", inplace=True)
+        df_events.query(
+            f"(combo_type < {combo_types[-1]}) & (magic_stereo == True)", inplace=True


taking into account the other problem that we had with the combo_type it might be safer to access this combination by name "M1_M2"

jsitarek · 2023-12-13T07:34:34Z

-    elif event_type == "software_only_3tel":
-        df_events.query("combo_type == 3", inplace=True)
+    elif event_type == "software_6_tel":
+        df_events.query(f"combo_type < {combo_types[-1]}", inplace=True)


jsitarek · 2023-12-13T07:34:40Z


    elif event_type == "magic_only":
-        df_events.query("combo_type == 0", inplace=True)
+        df_events.query(f"combo_type == {combo_types[-1]}", inplace=True)


jsitarek · 2023-12-13T07:36:37Z

        If the input event type is not known
    """

+    TEL_NAMES, TEL_COMBINATIONS = telescope_combinations(config)


the same 6 lines are also in load_mc_dl2_data_file
better to extract them in a separate function to make sure that the same code is used for data and MCs

the only difference is that in real data we do not ask for magic_stereo == True in the 'software' because standard MAGIC + LST-1 data are always collected with MAGIC stereo trigger, but we can take this into account easily

jsitarek · 2023-12-13T07:47:59Z

+        event_data.query(f"combo_type == {combo_types[-1]}", inplace=True)

    elif event_type == "hardware":
        logger.warning(


hardware trigger is already implemented in ctapipe_io_magic

jsitarek · 2023-12-13T07:52:36Z

-for the events of the any 2-tel combinations except the MAGIC-stereo
-combination at the moment. The "software_only_3tel" type allows for only
-the events of the 3-tel combination. The "magic_only" type allows for
+The "software_3tels_or_more" type stands for the software event


if we modify the types following my earlier comment they need to be updated here as well.

jsitarek · 2023-12-13T07:54:36Z

-    combo_type = list(TEL_COMBINATIONS.values()).index(tel_ids)
-    df_events = event_data.query(f"combo_type == {combo_type}")
+    # Extract the events with the same telescope ID
+    df_events = event_data.query(f"tel_id == {tel_ids[0]}")


I do not understand why do you always take the first tel_id here

Because the tel_ids list (i.e., the dictionary in 'estimator') only contains one element (we create one RF file per telescope and this 'list' will contain the ID of the telescope corresponding to the used RF file). So we take always the first because ti is the only tel_id

Elisa-Visentin · 2024-01-22T07:50:20Z

Due to the large number of conflict with the master (and the 'timescale' of the commits both here and in the master, which will probably cause a mess if we try to merge them), I will open a new branch from the current master (ctapipe 0.19) to implement 'manually' the changes in the RFs; we can then merge this new branch. This PR will be kept open as a draft to allow us to solve the open conversations

Elisa-Visentin added 2 commits December 5, 2023 15:26

last new scripts

24a9459

fixes

f2d39a9

numpydoc

a8b77de

Elisa-Visentin marked this pull request as ready for review December 6, 2023 07:25

Elisa-Visentin and others added 2 commits December 6, 2023 08:28

Doc fixes (astropy)

e36a237

Merge branch 'master' into Torino

ede00c3

aleberti approved these changes Dec 8, 2023

View reviewed changes

Comment thread magicctapipe/scripts/lst1_magic/lst1_magic_train_rfs.py Outdated

Comment thread magicctapipe/scripts/lst1_magic/lst1_magic_train_rfs.py

Comment thread magicctapipe/scripts/lst1_magic/lst1_magic_create_irf.py Outdated

aleberti requested a review from jsitarek December 8, 2023 15:33

Elisa-Visentin added 2 commits December 8, 2023 17:16

Update lst1_magic_train_rfs.py

d7699f0

Update lst1_magic_create_irf.py

1c86f0d

Elisa-Visentin mentioned this pull request Dec 11, 2023

URGENT: combo_type evaluation: fixed bug #181

Merged

jsitarek requested changes Dec 13, 2023

View reviewed changes

Elisa-Visentin added 5 commits December 27, 2023 18:59

query + minor fixes

f44f39c

query types

4346e25

Merge branch 'master' into Torino

66f5ec5

doc

5bf4aaa

merge issues

e5d8a1a

aleberti linked an issue Jan 19, 2024 that may be closed by this pull request

Optimization of RF structure for MAGIC+LST1 analysis #27

Open

aleberti added enhancement New feature or request new functionality For new functionalities optimization For code optimization labels Jan 19, 2024

Elisa-Visentin marked this pull request as draft January 22, 2024 07:50

Elisa-Visentin mentioned this pull request Jan 22, 2024

new RF implementation #194

Open

aleberti removed a link to an issue Feb 17, 2024

Optimization of RF structure for MAGIC+LST1 analysis #27

Open

Elisa-Visentin mentioned this pull request Apr 28, 2026

new features needed #332

Open

3 tasks

Uh oh!

Conversation

Elisa-Visentin commented Dec 5, 2023

Uh oh!

aleberti commented Dec 5, 2023

Uh oh!

codecov Bot commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Elisa-Visentin commented Dec 5, 2023 via email

Uh oh!

Elisa-Visentin commented Dec 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aleberti commented Dec 8, 2023

Uh oh!

jsitarek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Elisa-Visentin commented Jan 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Dec 5, 2023 •

edited

Loading