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

added support for processing MARS MC _Y_ files #45

Merged
merged 6 commits into from
Apr 27, 2022
Merged

Conversation

jsitarek
Copy link
Collaborator

No description provided.

@YoshikiOhtani
Copy link
Collaborator

Hi @jsitarek, thanks for your modifications, in general it looks fine for me, as you already succeeded in processing MAGICSoft MCs with this changes. Still, I have some ideas to make it more automatically as I already commented, and so if you don't mind I would like to push a new commit with my modifications on this script, is it fine for you?

@jsitarek
Copy link
Collaborator Author

Hi @YoshikiOhtani do you mean a new commit in this branch, or to make a separate PR from the main branch?

@YoshikiOhtani
Copy link
Collaborator

Hi @jsitarek, I meant a commit to this branch, but if you prefer I can make a new PR after merging this branch to master.

@jsitarek
Copy link
Collaborator Author

I think it is best if you commit to this branch (just wanted to make sure that this is what you actually meant).
If you are going to make some clean up, I had to copy a container that you use in another script (and rename it to ...MC), maybe this container can be defined somewhere in the main files of the module (but either way this will be somewhat temporal, because in the end we will need to make the format consistent with ctapipe standard containers)

… but taken from ctapipe_io_magic

set also magic_stereo to true when reading MCs, because ctapipe_io_magic reads only stereo MC events
@jsitarek
Copy link
Collaborator Author

Hi @YoshikiOhtani
I made two minor changes to this branch, I hope it does not interfere with your changes

@YoshikiOhtani
Copy link
Collaborator

YoshikiOhtani commented Apr 26, 2022

I finished cleaning up the code. Let me briefly summarize the changes here:

  • It now automatically checks the data type and processes it differently between real and MC data. So one does not need to give "--use-mc" argument.
  • It stores the telescope positions obtained from the MAGICEventSource, but the telescope IDs are reset for the convenience of the combined analysis with LST-1 (tel_id = 1).
  • Removed the author name because it is expected that more people may contribute on this script in future
  • Updated the merge_hdf_file.py so that it can be used for MAGICSoft MC data files

I tested the updated scripts to some of MAGIC data and it worked fine. It would be great if someone could check them and give me some comments.

By the way it accidentally contains a bug fix commit which is not related to this discussion, but I see that the bug has been already fixed by Elli and so please ignore the commit.

@YoshikiOhtani
Copy link
Collaborator

Since I would like to make another pull request about dead time calculations based on these changes, let me merge this to the master. I confirmed that the updated script works fine so I hope it doesn't make any problems.

@YoshikiOhtani YoshikiOhtani merged commit 365871b into master Apr 27, 2022
@YoshikiOhtani YoshikiOhtani deleted the mc_Y_to_dl1 branch April 27, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants