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

Script to recompute DL1b parameters from a DL1 file #222

Merged
merged 6 commits into from
Nov 21, 2019

Conversation

vuillaut
Copy link
Member

  • Read a HDF5 DL1 file, recompute parameters based on calibrated images and pulse times and a config file
  • write a new HDF5 file
  • Updated parameters are : Hillas paramaters, wl, r, leakage, n_islands, intercept, time_gradient

This is not the best approach since it recodes the computation of the parameters.
A better approach would be to code an EventSource for DL1 (but this will come with ctapipe DL1 Tools) and use the same pipeline once we have calibrated images, no matter their origin.

@vuillaut vuillaut added the ready for review Pull request is ready for review label Nov 19, 2019
@morcuended
Copy link
Member

Maybe you should rename the script to lstchain_dl1ab.py since will work with MC and real data

Copy link
Member

@morcuended morcuended left a comment

Choose a reason for hiding this comment

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

I have tested it with a dl1 file and it's working fine. I get new files with and without images. When I save the image table it takes quite long with respect to the no-image option though. However, the image is not recalculated, right? Shouldn't it be just the same? So I expected the run time to be more or less the same.

@vuillaut
Copy link
Member Author

I have tested it with a dl1 file and it's working fine. I get new files with and without images. When I save the image table it takes quite long with respect to the no-image option though. However, the image is not recalculated, right? Shouldn't it be just the same? So I expected the run time to be more or less the same.

It should be more or less the same yes, the only difference is the copy. So depending on the size of the data, your specs, etc... time may vary but I don't expect such a huge difference.
Of what order of magnitude are we talking here?

@morcuended
Copy link
Member

morcuended commented Nov 20, 2019

I ran the script in parallel using srun. These are the runtimes and memory usage I got with including image (first) and not including it (latter) for a file with 100 events.

    ReqMem     MaxRSS     AveRSS    Elapsed 
---------- ---------- ---------- ---------- 
    8000Mc    243808K    193928K   00:03:47 
    8000Mc    149600K    149600K   00:01:40

I would like to cross-check the runtimes but the IT container is down right now

@vuillaut
Copy link
Member Author

I ran the script in parallel using srun. These are the runtimes and memory usage I got with including image (first) and not including it (latter) for a file with 100 events.

    ReqMem     MaxRSS     AveRSS    Elapsed 
---------- ---------- ---------- ---------- 
    8000Mc    243808K    193928K   00:03:47 
    8000Mc    149600K    149600K   00:01:40

I would like to cross-check the runtimes but the IT container is down right now

Weird, I don't have such difference on my laptop:
For a 676MB DL1 file containing ~11.000 events:
With images:

real    0m52.209s
user    1m22.858s
sys     0m9.207s

Without images:

real    0m48.386s
user    1m21.733s
sys     0m7.502s

@morcuended
Copy link
Member

I'm probably doing something wrong then. Will check it again on my local machine

@morcuended
Copy link
Member

morcuended commented Nov 21, 2019

I'm probably doing something wrong then. Will check it again on my local machine

My bad. I checked it again and the time values make sense now. These are for a whole subrun (53000 events):

Keeping images

real    2m16.617s
user    2m22.056s
sys     1m38.740s

Without images

real    2m9.230s
user    2m18.673s
sys     1m35.348s

The content of both files is OK as well. Let's merge it.

@rlopezcoto
Copy link
Contributor

thanks @morcuended for the quick review and @vuillaut for the code!

@rlopezcoto rlopezcoto merged commit 902f957 into cta-observatory:master Nov 21, 2019
@vuillaut
Copy link
Member Author

Thanks for the tests @morcuended !

@vuillaut vuillaut deleted the dl1ab branch November 21, 2019 16:38
@labsaha labsaha mentioned this pull request Nov 21, 2019
2 tasks
@vuillaut vuillaut mentioned this pull request Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants