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

Coincidence method for the MAGIC broken timestamp data #231

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

Ryo-Imazawa
Copy link

@Ryo-Imazawa Ryo-Imazawa commented May 30, 2024

We made a new function find_offset, which find and output the time dependent time offset value between MAGIC and LST (as numpy binary files). The lst1_magic_find_time_offset.py is the script for make it each subrun set.
We added the new function to magicctapipe and make changes for lst1_magic_event_coincidence.py to load the time offset files and correct the MAGIC timestamp (use --input-dir_toff option for activate it). To find the corresponding subrun files between MAGIC and LST each other and input it to the lst1_magic_event_coincidence.py, we made a script launch_coincicence_for_brokenGPS.py. The usage is below.

== Make time offset search ==

Usage:
$ python lst1_magic_find_time_offset.py data

If you have some runs the input data directory should be separated for each run and use them to save the time, as like,
data1/LST/
dl1_LST-1.Run17220.0000.h5 dl1_LST-1.Run17220.0001.h5 ...
data1/MAGIC/
dl1_MAGIC.Run05114133.001.h5 dl1_MAGIC.Run05114133.002.h5 ...

data2/LST/
dl1_LST-1.Run17221.0000.h5 dl1_LST-1.Run17221.0001.h5 ...
data2/MAGIC/
dl1_MAGIC.Run05114134.001.h5 dl1_MAGIC.Run05114134.002.h5 ...

$ python lst1_magic_find_time_offset.py data1
In parallel,
$ python lst1_magic_find_time_offset.py data2

It will take a few tens of minutes to make each npy file. Basically, each MAGIC run contains ~10 subruns and so this step takes a few hours to be finished.

== Run the lst_magic_event_coincidence.py ==

Run the coincidence script using npy files made before step.
The input files should be corresponding subrun with each other. you can use attached file ($ python launch_coincicence_for_brokenGPS.py.

Then you will find many coincided dl1 files, you can merge them with merge_hdf_files.py for the next dl1 to dl2 step.

@jsitarek
Copy link
Collaborator

thank you @Ryo-Imazawa
linter and pyflakes show some errors (like defined variables that are never used), can you run precommit before we review the PR?

@Ryo-Imazawa
Copy link
Author

thank you @Ryo-Imazawa linter and pyflakes show some errors (like defined variables that are never used), can you run precommit before we review the PR?

Thank you, I will check what happened and modify it.

@Ryo-Imazawa
Copy link
Author

Dear reviewers,

We confirmed that the pre-commit shows all Passed.

The previous script "lst1_magic_find_time_offset.py" and "launch_coincidence_for_brokenGPS.py" are merged to "coincident_brokenGPS.py". So you can find the time offset and make coincident dl1 files as below,

Usage:
$ python coincident_brokenGPS.py data/

The data/ directory should contain LST and MAGIC directories and the sub-run files should be put under their directories, as below,

data/LST/
dl1_LST-1.Run17220.0000.h5 dl1_LST-1.Run17220.0001.h5 ...
data/MAGIC/
dl1_MAGIC.Run05114133.001.h5 dl1_MAGIC.Run05114133.002.h5 ...

It is recommended that data directories are separated for each run to save time.

magicctapipe/io/io.py Outdated Show resolved Hide resolved
magicctapipe/io/io.py Outdated Show resolved Hide resolved
magicctapipe/io/io.py Outdated Show resolved Hide resolved
magicctapipe/io/io.py Outdated Show resolved Hide resolved
time_offset_center - 1.6e-6,
time_offset_center + 1.6e-6,
)
time_select = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

what you do in each step is very similar to what you do at the beginning of the function with initial_time_offset None, maybe you can extract this into a separate function, or do the first case as 1 iteration case of multiple iterations?

t0_sec = [Decimal(str(time)) for time in [t0]]
t0_sec = np.array(t0_sec) * SEC2NSEC
t0_sec = u.Quantity(t0_sec, unit="ns", dtype=int)
timestamps_lst = timestamps_lst_arr[tel_id - 2] - t0_sec
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is using tel_id = 2 and 3 for MAGIC-I and MAGIC-II, better to read it from the config

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.

thank you @Ryo-Imazawa I went through the code and left some comments.
Additionally there are some merge conflicts that need to be solved

@Ryo-Imazawa
Copy link
Author

Thank you @jsitarek for your kind comments. I will check it and revise them.

@aleberti aleberti added the new functionality For new functionalities label Sep 3, 2024
magic_run, lst_run = print_gti(df_magic, df_lst)
df = pd.DataFrame({"magic_run": magic_run, "lst_run": lst_run})

try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

with makedirs you do not need to put it in try except block

@jsitarek
Copy link
Collaborator

jsitarek commented Oct 2, 2024

Hi @Ryo-Imazawa thanks for the improvements
another round of checks. I closed some comments that seem to be resolved now, and also added some comments to the remaining ones. I also saw that you put my comments in the text, those will also need to be removed before merging the code.

@Ryo-Imazawa
Copy link
Author

Hi @jsitarek . Thank you for your response.
As you say, some improvements have been addressed and now I'm trying to checking to see if it works correctly.

Comments written for own notes will be crossed out later. When I finish the modifications I will upload the code.

@jsitarek
Copy link
Collaborator

jsitarek commented Oct 2, 2024

ok, just let me know when you finish, I will make another quick look and we merge.

Note also that we got in the meantime another branch conflict

@Ryo-Imazawa
Copy link
Author

Hi @jsitarek . I fixed the minor bugs and the conflicts.
Please review to make merge the branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new functionality For new functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants