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

Lidar corr #95

Draft
wants to merge 40 commits into
base: master
Choose a base branch
from
Draft

Lidar corr #95

wants to merge 40 commits into from

Conversation

nzywucka
Copy link
Collaborator

No description provided.

…Container() class and parse_laser_info() function.
…ports. Filtering for the unique reports has been added.
Reading the unique reports has been improved
…arameters in the container.

All the parameters in the container are stored in the list of parameters now.
�# Please enter the commit message for your changes. Lines starting
@@ -92,6 +94,68 @@
DATA_MAGIC_LST_TRIGGER: EventType.SUBARRAY,
}

class ReportLaserContainer(Container):
""" Container for Magic laser parameters """
UniqueID = Field(List[Any], 'No.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should merge the latest changes from the lidar_test branch here

def magic_reports_reading(self, input_file, process_run=False):
logger.info(f"\nInput file: {input_file}")
event_source = MAGICEventSource(input_file, process_run=process_run)
is_simulation = event_source.is_simulation
Copy link
Collaborator

Choose a reason for hiding this comment

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

there are no reports in simulations

is_simulation = event_source.is_simulation
logger.info(f"\nIs simulation: {is_simulation}")

obs_id = event_source.obs_ids[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this needed, looks like debugging


laser = event_source.laser

for key, report_list in laser.items():
Copy link
Collaborator

Choose a reason for hiding this comment

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

this does not fit the current scheme of writing the reports: list rather then a dictionary
and the whole loop is not needed because it just shown a debug message

return report

def get_c0(self, time, zenith, range_max):
filename = '/home/zywuckan/ctapipe_io_magic/ctapipe_io_magic/params.txt'
Copy link
Collaborator

@jsitarek jsitarek Apr 3, 2024

Choose a reason for hiding this comment

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

variable, value = line.split("=")
variable = variable.strip()
value = value.split("#")[0].strip()
if variable == "stime21":
Copy link
Collaborator

Choose a reason for hiding this comment

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

all this code just to initialize a bunch of variables to -1 ??
and you should use arrays of parameters rather then stimeXX

gkHWSwitchV1to4 = int(value)

C_0 = gkC_0Period1 # absolute calibration at beginning of 2013
if stime21 > 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

all of those stimes are read in as -1 so neither of the conditions later on will trigger


return report

def get_c0(self, time, zenith, range_max):
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a doc string explaining the parameters and units

range_max_clouds = (range_max - 100.) * np.cos(zenith_radians)
return C_0

def apply_time_dependent_corrections(self, datime, c0, coszd, case_index=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

case_index is not used in the function

add doc string with explanation of parameters


def apply_time_dependent_corrections(self, datime, c0, coszd, case_index=None):
switch_times = {}
filename = '/home/zywuckan/ctapipe_io_magic/ctapipe_io_magic/switch_times.txt'
Copy link
Collaborator

Choose a reason for hiding this comment

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

use relative paths

with open(filename, 'r') as file:
switch_time = None
for line in file:
if line.startswith("Switch Time:"):
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 a bit dangerous because it depends on the sequence of switch time and correction parameters lines.
Why not simply put both in the same line, first the date and then correction parameters
maybe also drop the lines starting with # (like you do in params.txt) and add a comment in the file

if switch_time:
switch_times[switch_time] = correction_params

stimes = [(datime - switch_time).total_seconds() for switch_time in switch_times]
Copy link
Collaborator

Choose a reason for hiding this comment

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

stimes and AlphaFit_corr are not use and not returned by the function

stimes = [(datime - switch_time).total_seconds() for switch_time in switch_times]
Alphafit_corr = 0.0

def apply_zenith_correction(self, c0, coszd):
Copy link
Collaborator

Choose a reason for hiding this comment

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

docstring

return True

def apply_azimuth_correction(self, c0, zenith, azimuth):
filename = '/home/zywuckan/ctapipe_io_magic/ctapipe_io_magic/zd_az_corr.txt'
Copy link
Collaborator

Choose a reason for hiding this comment

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

relative paths

c0 += 2 * zd_corr
return True

def apply_azimuth_correction(self, c0, zenith, azimuth):
Copy link
Collaborator

Choose a reason for hiding this comment

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

docstring

with open(filename, 'r') as file:
rule = {}
for line in file:
if line.startswith("Rule"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

add skipping lines with #

if rule:
rules.append(rule)
rule = {}
elif line.startswith("Zenith Threshold:"):
Copy link
Collaborator

@jsitarek jsitarek Apr 3, 2024

Choose a reason for hiding this comment

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

I think it would be simpler if you use only threshold min threshold max, but if it is not filled you initialize it with 0 and 90 deg

return True
return False

def get_bin_width(self, ifadc, bgsamples, ncollapse):
Copy link
Collaborator

Choose a reason for hiding this comment

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

docstring

return False

def get_bin_width(self, ifadc, bgsamples, ncollapse):
filename = '/home/zywuckan/ctapipe_io_magic/ctapipe_io_magic/params.txt'
Copy link
Collaborator

Choose a reason for hiding this comment

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

relative paths

also this file seems to be opened in multiple functions

if bgsamples <= gkBGSamplesV1to4:
return 0.0
if ifadc < 128:
return - bgsamples * gkIntegrationWindow * self.get_bin_width(ifadc, bgsamples-1, ncollapse)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope this function will not be executed for every event, because it is doing file operations

return ((phecounts - self.get_bin_width(ifadc, bgsamples, ncollapse) * bg) * hwcorr
/ self.get_bin_width(ifadc, bgsamples, ncollapse))

def main():
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 fine for tests, but ultimately you should just execute the relevant functions from init.py file and store the calibrated LIDAR information in the data structure (like you do with uncalibrated)

…rameters

azimuth_conditions.txt: parameters to perform azimuth corrections
switch_times.txt and switchtimes1.txt: parameters to perform time corrections
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 24.90222% with 576 lines in your changes missing coverage. Please review.

Project coverage is 63.82%. Comparing base (393d187) to head (5221ce4).
Report is 61 commits behind head on master.

Files with missing lines Patch % Lines
ctapipe_io_magic/laser_params_corr_test.py 6.83% 327 Missing ⚠️
ctapipe_io_magic/laser_test_new.py 0.00% 247 Missing ⚠️
ctapipe_io_magic/__init__.py 98.63% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master      #95       +/-   ##
===========================================
- Coverage   89.57%   63.82%   -25.75%     
===========================================
  Files          10       13        +3     
  Lines        1103     1935      +832     
===========================================
+ Hits          988     1235      +247     
- Misses        115      700      +585     

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

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