Update MAGIC-LST scripts to ctapipe v0.12.0 #6

YoshikiOhtani · 2022-01-12T13:05:06Z

Here the combined analysis scripts are updated to be used in the ctapipe v0.12.0 environment.

Replace get_key_if_exists with dict.get

…tory/magic-cta-pipe into dev-yoshiki Conflicts: lst1_magic/magic_data_cal_to_dl1.py magicctapipe/scripts/lst1_magic_real/check_coincidence_overview.py magicctapipe/scripts/lst1_magic_real/check_pointing_directions.py magicctapipe/scripts/lst1_magic_real/get_common_obs_info.py magicctapipe/scripts/lst1_magic_real/lst1_magic_dl1_to_dl2.py magicctapipe/scripts/lst1_magic_real/lst1_magic_event_coincidence.py magicctapipe/scripts/lst1_magic_real/lst1_magic_mc_dl0_to_dl1.py magicctapipe/scripts/lst1_magic_real/lst1_magic_stereo_reco.py magicctapipe/scripts/lst1_magic_real/lst1_magic_train_rfs.py magicctapipe/scripts/lst1_magic_real/notebooks/check_data_quality.ipynb magicctapipe/scripts/lst1_magic_real/notebooks/check_sensitivity_from_data.ipynb magicctapipe/scripts/lst1_magic_real/notebooks/check_spectrum.ipynb magicctapipe/utils/merge_hdf_files.py magicctapipe/utils/my_functions.py utils/__init__.py

review-notebook-app · 2022-01-12T13:05:09Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

aleberti · 2022-01-13T08:31:15Z

magicctapipe/scripts/lst1_magic_real/data/config.yaml

@@ -43,41 +73,82 @@ event_coincidence:


 stereo_reco: 
-    subarray: '/fefs/home/yoshiki.ohtani/software/magic-cta-pipe/lst1_magic/data/subarray_lst1_magic.pkl'
-    quality_cuts: (intensity > 50) & (width > 0)
+    subarray: '/home/yoshiki.ohtani/software/magic-cta-pipe/magicctapipe/scripts/lst1_magic_real/data/subarray_lst1_magic.pickle'


I think that is better to save the subarray information together with the DL1 file(s) produced by the event coincidence script. If you have the SubarrayDescription object (as saved in the pickle file), you can define it in the event coincidence script and use the SubarrayDescription.to_hdf method to write it into the HDF5 file (if I understood correctly, saving the subarray in the HDF5 file should be done before saving anything else).
This would avoid having to specify the path to the pickle file, and instead reading everything from the file at once.

Hm, OK, I think I can define the subarray object inside the stereo reconstruction script. I'm implementing this solution now.

By the way saving the subarray after saving others, for example event dl1 parameters works without any problems. Probably it was updated?

aleberti · 2022-01-13T08:37:48Z

magicctapipe/scripts/lst1_magic_real/lst1_magic_mc_dl0_to_dl1.py

+    magic_stereo = Field(-1, 'True if M1 and M2 are triggered')
+
+
+class SimInfoContainer(Container):


Why not using the SimulationConfigContainer from ctapipe instead of defining a new one? SimulationConfigContainer has more members, so you will not use them all, but I think we should try to use already defined things (especially if already in ctapipe)

Ok, now I see that you extract source.simulation_config, which indeed is a SimulationConfigContainer, and then fill the custom container. Since the simulation configuration is not a big container (anyway just one per run), I would write directly source.simulation_config into the HDF5 file.

Thanks for your comment, and indeed I agree with you, I will change it.

aleberti · 2022-01-13T08:53:15Z

magicctapipe/scripts/lst1_magic_real/lst1_magic_stereo_reco.py

        if stereo_params.az < 0:
-            stereo_params.az = stereo_params.az + u.Quantity(2*np.pi, u.rad)
+                stereo_params.az += u.Quantity(360, u.deg)


here the indentation is wrong, should be one tab less

thanks I modified it

aleberti · 2022-01-13T10:00:57Z

setup.py

@@ -22,11 +22,11 @@
    install_requires=[
        'astropy>=4.0.5,<5',
        'ctapipe~=0.12.0',
-        'ctapipe_io_magic~=0.3.0',
+        'ctapipe_io_magic',


Was this an issue for you? Anyway this will be changed when we will merge into master and I will make a new version of ctapipe_io_magic (v0.4.0, which I will create from the current master branch, the one working with ctapipe v0.12)

Yes there was an issue for some reason, anyway the version 0.3.0 is for ctapipe v0.8.0 and so I modified it. Would be great if you create v0.4.0, and then I will modify it again.

Ok, I can do it now. I will check if I need to update a bit the README.md and then create tag and release.

Ok, it took a bit, but now ctapipe_io_magic was tagged with version 0.4.0. Also, it can be installed with pip without cloning the repo, since now I uploaded the module in PyPI.

Thanks! Ah so it means now one doesn't need to clone both ctapipe and ctapipe_io_magic, and instead just need to create an environment, clone only magic-cta-pipe and type pip install .?

To have everything working as you say, indeed we would need to create an environment.yml file, which will install ctapipe 0.12 (and dependencies), ctapipe_io_magic 0.4.0 (and dependencies) and finally magic-cta-pipe

I see, is it possible for you to create an environment.yml file? I think it would be better and easy way for the students to install the pipeline if they need to do it by themself (in the local machine, another computer, etc)

Sure, I can do it. I will do a commit in the update-ctapipe-v0.12 branch.

Thanks a lot!

aleberti

Ok, I think I checked everything, and put my comments, and some you already replied. So at that point I will check them again. Thanks for updating the scripts!

…ions

…tory/magic-cta-pipe into dev-yoshiki

YoshikiOhtani · 2022-01-13T16:01:03Z

OK so I think I updated all the points that Alessio mentioned and pushed the commits, but have not yet tested them. I will test them tomorrow (already today in Japan :)) and will let you know once it is done. Then if there are not any further comments I think we can merge it.

…-pipe into dev-yoshiki Conflicts: magicctapipe/utils/__init__.py

… file.

YoshikiOhtani · 2022-01-14T15:37:55Z

Sorry for taking time but I found some mistakes in the scripts about the subarray descriptions, and so I modified them. I also updated the script for merging HDF files. I tested the script and confirmed that now it works fine.

YoshikiOhtani · 2022-01-14T15:49:21Z

@aleberti, if it is fine for you, could you merge this branch to update-ctapipe-v0.12, and merge update-ctapipe-v0.12 to master, please? And probably create a new tag (v0.1.0 ?) for the current version?

aleberti · 2022-01-14T16:03:52Z

Sure, give me some time and I do it :)

YoshikiOhtani · 2022-01-14T16:18:41Z

Thanks a lot! I will delete my branch then…

aleberti · 2022-01-14T16:24:14Z

Sure, in any case I have to do a second tag, because I need to add some files to build the package to upload it to PyPI.

Yoshiki Ohtani and others added 7 commits January 5, 2022 06:32

updates for the combined analysis, ctapipe v0.8.0, lstchain v0.7.5

46e8070

minor

8bdc199

Replace get_key_if_exists with dict.get

3d8c131

Merge pull request #4 from cta-observatory/get

0afdcd2

Replace get_key_if_exists with dict.get

Add script to download test data.

f2969f5

Remove requirement of ctapipe_io_magic version

42c7e75

YoshikiOhtani requested a review from aleberti January 12, 2022 13:05

Minor modifications

f941e1f

aleberti reviewed Jan 13, 2022

View reviewed changes

aleberti requested changes Jan 13, 2022

View reviewed changes

YoshikiOhtani added 2 commits January 13, 2022 15:48

Store/load subarray description to/from HDF file, and minor modificat…

1b5de6c

…ions

Merge branch 'update-ctapipe-v0.12' of https://github.com/cta-observa…

777f649

…tory/magic-cta-pipe into dev-yoshiki

YoshikiOhtani added 2 commits January 14, 2022 05:42

Merge branch 'master' of https://github.com/cta-observatory/magic-cta…

257708b

…-pipe into dev-yoshiki Conflicts: magicctapipe/utils/__init__.py

Bugfix of subarray description, and update the script for merging HDF…

3e6fa8a

… file.

aleberti approved these changes Jan 14, 2022

View reviewed changes

aleberti merged commit 75f5e84 into update-ctapipe-v0.12 Jan 14, 2022

YoshikiOhtani deleted the dev-yoshiki branch January 14, 2022 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MAGIC-LST scripts to ctapipe v0.12.0 #6

Update MAGIC-LST scripts to ctapipe v0.12.0 #6

YoshikiOhtani commented Jan 12, 2022 •

edited

Loading

review-notebook-app bot commented Jan 12, 2022

aleberti Jan 13, 2022

YoshikiOhtani Jan 13, 2022

YoshikiOhtani Jan 13, 2022

aleberti Jan 13, 2022

aleberti Jan 13, 2022

YoshikiOhtani Jan 13, 2022

aleberti Jan 13, 2022

YoshikiOhtani Jan 13, 2022

aleberti Jan 13, 2022

YoshikiOhtani Jan 13, 2022

aleberti Jan 13, 2022

aleberti Jan 13, 2022

YoshikiOhtani Jan 13, 2022

aleberti Jan 13, 2022

YoshikiOhtani Jan 13, 2022

aleberti Jan 13, 2022

YoshikiOhtani Jan 13, 2022

aleberti Jan 13, 2022

aleberti left a comment

YoshikiOhtani commented Jan 13, 2022 •

edited

Loading

YoshikiOhtani commented Jan 14, 2022

YoshikiOhtani commented Jan 14, 2022

aleberti commented Jan 14, 2022

YoshikiOhtani commented Jan 14, 2022

aleberti commented Jan 14, 2022

		magic_stereo = Field(-1, 'True if M1 and M2 are triggered')


		class SimInfoContainer(Container):

Update MAGIC-LST scripts to ctapipe v0.12.0 #6

Update MAGIC-LST scripts to ctapipe v0.12.0 #6

Conversation

YoshikiOhtani commented Jan 12, 2022 • edited Loading

review-notebook-app bot commented Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleberti left a comment

Choose a reason for hiding this comment

YoshikiOhtani commented Jan 13, 2022 • edited Loading

YoshikiOhtani commented Jan 14, 2022

YoshikiOhtani commented Jan 14, 2022

aleberti commented Jan 14, 2022

YoshikiOhtani commented Jan 14, 2022

aleberti commented Jan 14, 2022

YoshikiOhtani commented Jan 12, 2022 •

edited

Loading

YoshikiOhtani commented Jan 13, 2022 •

edited

Loading