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 for gain selection (R0 to R0G), for data taken in ADHF format … #1198

Merged
merged 38 commits into from
Feb 23, 2024

Conversation

moralejo
Copy link
Collaborator

(i.e. for data taken from July 2023).
Still being tested.

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (050056a) 72.67% compared to head (39a5027) 72.87%.
Report is 2 commits behind head on main.

Files Patch % Lines
lstchain/scripts/lstchain_r0_to_r0g.py 90.16% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1198      +/-   ##
==========================================
+ Coverage   72.67%   72.87%   +0.19%     
==========================================
  Files         132      133       +1     
  Lines       13655    13797     +142     
==========================================
+ Hits         9924    10054     +130     
- Misses       3731     3743      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moralejo
Copy link
Collaborator Author

moralejo commented Dec 20, 2023

Note: the gain - reduced files seem to be readable and apparently the data inside is correct. But I get the following warning upon reading them with ctapipe_io_lst:
WARNING: Unexpected extra padding at the end of the file. This padding may not be preserved when saving changes. [astropy.io.fits.header]
@maxnoe : Is this a problem? Can we avoid the warning altogether?

Note: I saw this with the EvB5 file I tested, but not with the EvB6 file (not sure the reason is the different version, though)

@morcuended
Copy link
Member

Will this gain selector also work with EVB6 data?

@moralejo
Copy link
Collaborator Author

moralejo commented Dec 21, 2023

Will this gain selector also work with EVB6 data?

It should, because the format is the same (ADHF). Will try it.

@moralejo
Copy link
Collaborator Author

moralejo commented Dec 21, 2023

Will this gain selector also work with EVB6 data?

It should, because the format is the same (ADHF). Will try it.

No, it does not work... 'File' object has no attribute 'CameraConfig'
It seems that in EvB6 data it is called CameraConfiguration

@moralejo
Copy link
Collaborator Author

No, it does not work... 'File' object has no attribute 'CameraConfig' It seems that in EvB6 data it is called CameraConfiguration

Fixed now, there are indeed a few differences I was unaware of between EvB5 and EvB6 ADHF data.

@moralejo moralejo marked this pull request as ready for review January 8, 2024 08:27
@moralejo moralejo requested a review from maxnoe January 8, 2024 08:27
@maxnoe
Copy link
Member

maxnoe commented Jan 8, 2024

What is "ADHF" and where does this acronym come from?

@moralejo
Copy link
Collaborator Author

moralejo commented Jan 8, 2024

What is "ADHF" and where does this acronym come from?

I am afraid I made it up, sorry. The original wording (in the ELOGs) is "ADH-ZFW", butI am not sure either whether that is official or not.

@maxnoe
Copy link
Member

maxnoe commented Jan 8, 2024

Ah ok. There are three major combinations of software versions I think.

They can be distinguished by looking at the EXTNAME and the PBFHEAD header keywords in the corresponding FITS HDUs.

  • EVBv5 + "old" ZFW (ZFitsWriter compiled from svn CamerasToACTL repository).

    This is the oldest combination and it uses the ProtoR1 protocol buffer definitions with the PBFHEAD keyword set to R1.Event etc.

In [6]: f = fits.open("/fefs/aswg/data/real/R0/20201120/LST-1.1.Run02962.0000.fits.fz")

In [7]: [hdu.name for hdu in f]
Out[7]: ['PRIMARY', 'CameraConfig', 'Events']

In [8]: f['CameraConfig'].header["PBFHEAD"]
Out[8]: 'R1.CameraConfiguration'

In [9]: f['Events'].header["PBFHEAD"]
Out[9]: 'R1.CameraEvent'
  • EVBv5 + ADH-ZFW ("new" ZFitsWriter compiled from git array-data-handler repository).

    This combination was used since mid of last year. It uses the same data model, but with a slightly different PBFHEAD keyword: ProtoR1.Event etc.

In [10]: f = fits.open("/fefs/aswg/data/real/R0/20230811/LST-1.1.Run13880.0000.fits.fz")

In [11]: [hdu.name for hdu in f]
Out[11]: ['PRIMARY', 'CameraConfig', 'Events']

In [12]: f['CameraConfig'].header["PBFHEAD"]
Out[12]: 'ProtoR1.CameraConfiguration'

In [13]: f['Events'].header["PBFHEAD"]
Out[13]: 'ProtoR1.CameraEvent'
  • EVBv6 + ADH-ZFW

    This is using the new official CTAO R1v1 format. The extname of the camera configuration changed, the TelescopeDataStream is new etc.:

In [14]: f = fits.open("/fefs/aswg/data/real/R0/20231215/LST-1.1.Run16120.0000.fits.fz")

In [15]: [hdu.name for hdu in f]
Out[15]: ['PRIMARY', 'DataStream', 'CameraConfiguration', 'Events']

In [16]: f['DataStream'].header["PBFHEAD"]
Out[16]: 'R1v1.TelescopeDataStream'

In [17]: f['CameraConfiguration'].header["PBFHEAD"]
Out[17]: 'CTAR1.CameraConfiguration'

In [18]: f['Events'].header["PBFHEAD"]
Out[18]: 'CTAR1.Event'

I notice the PBFHEAD is inconsistent... will report to dirk and julien

@maxnoe
Copy link
Member

maxnoe commented Jan 8, 2024

The best solution is to treat data with R1.Event and ProtoR1.Event identically to the existing tool and add new code for dealing with the EVBv6 data.

@moralejo
Copy link
Collaborator Author

moralejo commented Jan 8, 2024

The best solution is to treat data with R1.Event and ProtoR1.Event identically to the existing tool and add new code for dealing with the EVBv6 data.

I don't understand - what I do now is to check empirically whether I find "CameraConfig" or "CameraConfiguration" in the file - the latter seems to be the case for EvB6 data. Isn't that enough to distinguish the two cases we find since mid-2023?
There is no need for the program to work for EvB5 data EvB5+old ZFW data, which will were already gain-selected with your C++ code.

@maxnoe
Copy link
Member

maxnoe commented Jan 8, 2024

There is no need for the program to work for EvB5 data, which will were already gain-selected with your C++ code.

What identifies the new data format most clearly is the content of PBFHEAD, if it starts with R1v1 or CTAR1, it's the new official data format written by EVBv6.

That the extname of the CameraConfiguration HDU changed is more accidental...

The presence of the TelescopeDataStream is also new.

@maxnoe
Copy link
Member

maxnoe commented Jan 8, 2024

There is no need for the program to work for EvB5 data, which will were already gain-selected with your C++ code.

I thought there is, as the C++ code also doesn't work with case 2: EVBv5 plus ADH-ZFW. Isn't that the case?

@maxnoe
Copy link
Member

maxnoe commented Jan 8, 2024

However, as said above, that would be relatively easy to fix, so maybe the easiest is to fix the C++ code for the EVBv5 + ADH-ZFW case and let the code here only deal with EVBv6 aka CTAR1 aka R1v1?

@moralejo
Copy link
Collaborator Author

moralejo commented Jan 8, 2024

There is no need for the program to work for EvB5 data, which will were already gain-selected with your C++ code.

I thought there is, as the C++ code also doesn't work with case 2: EVBv5 plus ADH-ZFW. Isn't that the case?

Sorry, you are right.
I actually meant "There is no need for the program to work for EvB5 + old ZFW data, which will were already gain-selected with your C++ code".

@moralejo
Copy link
Collaborator Author

moralejo commented Jan 8, 2024

However, as said above, that would be relatively easy to fix, so maybe the easiest is to fix the C++ code for the EVBv5 + ADH-ZFW case and let the code here only deal with EVBv6 aka CTAR1 aka R1v1?

This script already works with both, using the "CameraConfig" vs. "CameraConfiguration" approach to identify what type of input data is being read in.

@moralejo
Copy link
Collaborator Author

moralejo commented Jan 8, 2024

About this warning:
WARNING: Unexpected extra padding at the end of the file. This padding may not be preserved when saving changes. [astropy.io.fits.header]

Is there any way of getting rid of it?

@maxnoe
Copy link
Member

maxnoe commented Jan 8, 2024

Is this happening for data where no event was written maybe? protozfits doesn't properly create / close the files if no event was ever written.

@moralejo
Copy link
Collaborator Author

moralejo commented Jan 23, 2024

Is this happening for data where no event was written maybe? protozfits doesn't properly create / close the files if no event was ever written.

No, there were events written out. But this only happened with the EvB v5 file I tested (not for EvB6 data, CTAR1 aka R1v1, which are the ones for which we will use this script)

@moralejo
Copy link
Collaborator Author

@maxnoe can we approve and merge this? If we get it in the next release we can start gain-selecting the data as part of the onsite automatic processing (hence reduce the disk-filling rate)

@maxnoe
Copy link
Member

maxnoe commented Jan 23, 2024

@moralejo We can remove the EVBv5 parts here, right? We will handle all the EVBv5 data with the old, c++ based tool, correct?

It also would be good to test this script (all lines are untested as of now). I can create a small sample with the zfitsrecompressor. Do you have an suitable run number for this?

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.

Apart from having a small test sample, I suggest testing it over an entire night.

environment.yml Show resolved Hide resolved
lstchain/scripts/lstchain_r0_to_r0g.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_r0_to_r0g.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_r0_to_r0g.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_r0_to_r0g.py Show resolved Hide resolved
@moralejo
Copy link
Collaborator Author

@moralejo We can remove the EVBv5 parts here, right? We will handle all the EVBv5 data with the old, c++ based tool, correct?

Well, apparently it works, and I think it does not harm to have it available in python form, even if we are not now using it for those data.

It also would be good to test this script (all lines are untested as of now). I can create a small sample with the zfitsrecompressor. Do you have an suitable run number for this?

I used Run13631.0042 (20230708), but any run should do.

@moralejo moralejo marked this pull request as draft February 20, 2024 17:45
@moralejo
Copy link
Collaborator Author

moralejo commented Feb 21, 2024

... and tests now fail because of non-standard input filename "first50" combined with use of lstchain functions which expect standard name for a raw fits.fz file :'-D
Solved

@moralejo moralejo marked this pull request as ready for review February 21, 2024 17:49
@moralejo moralejo marked this pull request as draft February 21, 2024 17:49
@moralejo moralejo marked this pull request as ready for review February 21, 2024 18:02
@moralejo moralejo requested a review from maxnoe February 21, 2024 18:02
@moralejo moralejo marked this pull request as draft February 22, 2024 14:37
@moralejo moralejo marked this pull request as ready for review February 22, 2024 15:10
@moralejo moralejo marked this pull request as draft February 22, 2024 15:38
@moralejo moralejo marked this pull request as ready for review February 22, 2024 15:52
@moralejo moralejo merged commit 5ea3010 into main Feb 23, 2024
9 checks passed
@moralejo moralejo deleted the gain_selector branch February 23, 2024 08:29
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.

4 participants