-
Notifications
You must be signed in to change notification settings - Fork 77
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
Real data analysis #201
Real data analysis #201
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems OK for me, a second check could be useful, though.
Just check the comment about the gain selection.
if is_simu: | ||
dl1_container.mc_energy = event.mc.energy.value | ||
dl1_container.log_mc_energy = np.log10(event.mc.energy.value * 1e3) | ||
|
||
dl1_container.log_intensity = np.log10(dl1_container.intensity) | ||
dl1_container.gps_time = event.trig.gps_time.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the pointing interpolation we need the event time. Since time information is not being written right now to the event.trig.gps_time field, from the time being I've been getting the event time as:
from astropy.time import Time
start_ntp = event.lst.tel[tel_id].svc.date
ntp_time = start_ntp + event.r0.tel[telescope_id].trigger_time
utc_time = Time(datetime.utcfromtimestamp(ntp_time))
gps_time = utc_time.gps
dl1_container.gps_time = gps_time
This should be modified back to the line you wrote when gps_time field is filled up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks.
I can integrate this change for real data.
Or you can PR into vuillaut:real_data
so we aggregate all the changes we need to make for real data analysis and add them at once to lstchain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did not know I could do that. I will work on your branch then.
In the end, do we want to have two separated output files corresponding to dl1a and dl1b data? @vuillaut @rlopezcoto @moralejo |
In my opinion, no, we want to follow CTA recommendations and produce files corresponding to the official structure. |
This would be easier than creating two h5 files already within the code |
Yes, that is what i was asking for. It is not just that image-less files are easier to move around (that is indeed one reason). It is also that we will want to have different DL1b versions (mostly, different cleaning approaches) out of the same DL1a. This can be done with several "versions" of DL1b inside a single file, or with separate DL1b files for each cleaning approach. |
Ok I see, for the second point, the current approach I have been applying is:
Would this approach be ok for you? I will make another PR for the dl1_to_dl1 script. |
writer.write(table_name=f'telescope/image/{tel_name}', | ||
containers=[event.dl0, tel, extra_im]) | ||
containers=[event.r0, tel, extra_im]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we write event.dl1
container with images and pulse times here instead of event.r0
or event.dl0
as you had before? This is something I doubted about when I was writing UCM script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The images and pulse time come from tel
. event.r0
includes info such as obs_id
and event_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true
os.makedirs(args.outdir, exist_ok=True) | ||
|
||
dl0_to_dl1.allowed_tels = {1, 2, 3, 4} | ||
output_filename = args.outdir + '/dl1_' + os.path.basename(args.infile).rsplit('.', 1)[0] + '.h5' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will produce filenames as: dl1_LST-1.1.Run01555.0000.fits.h5
. Maybe we should take out the fits part as well to have it like: dl1_LST-1.1.Run01555.0000.h5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, as right now it produces things like ***.simtel.h5
.
cons: that's a double file extension
pros: it keeps the info about origin
I don't have strong preferences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave it as it is then
I tested with real data and it works fine and produces a dl1 h5 file that contains:
So everything seems fine to me. One question though: Why real data h5 file still inherits the table |
Thanks a lot for giving it a try!
That's not intended indeed. Could you tell me the content of this table please? |
@morcuended don't bother, I found it. |
This is ok, though I guess we will do the image removal very often, to move around DL1b-only files.
I think the DL1 concept, containing both images and parameters is more designed for the final situation in which it will never contain all pixels of each camera, but just those present in DL0, or even just those surviving cleaning. In that case, the pixel info is not so much of a "ballast" when one is interested only in DL1b. |
Good! These are the tables contained in the h5 file after your modification:
Another question: should dl1 h5 file contain other tables with metadata? For example: [ 'instrument/subarray/layout', Or is it not needed at this data level anymore? |
it doesn't really matter- in HDF5 you can create a file that has internal symlinks to external files, so to the user it looks like a single file. I suggest that tools should expect them to be in one file (Even if in reality we use separate files, since we can use this linking trick). I'm not sure how efficient appending to an existing file is to add datasets, but I guess it should be possible to add the datasets to the existing file as well. |
For LST analysis it may not matter, but note that in a full analysis once we start using reconstructions like ImPACT or Model3D, we need to have both the images (though perhaps with a loose cleaning, like starndard + 3 extra dilation rings) and the parameters as well (for starting points to the fit). |
Hum you are right, I would prefer to have these tables in the file. I'll have a look. |
Sure, working on it |
These are the tables now
and they are being filled properly in the test I've done. So green light from my side for the merging |
thank you so much for the tests and review @morcuended |
So we go on and you @morcuended take over applying pointing modification discussed in #218 |
Some changes to include real data handling in the analysis.
The approach is to keep the same functions as much as possible for MC or real data.
There are discrepancies between real data and MC right now that need some handling though.
This is done with
if
condition on simu metadata.So this PR should not change anything for MC analysis (unit tests are passing so the pipeline works).
In the future, I hope we can progressively get rid of these conditions.