Adding a function to create a log file and save cloud correction para… #260
Conversation
…meters in the file
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #260 +/- ##
==========================================
- Coverage 77.23% 77.18% -0.06%
==========================================
Files 21 22 +1
Lines 2614 2621 +7
==========================================
+ Hits 2019 2023 +4
- Misses 595 598 +3 ☔ View full report in Codecov by Sentry. |
|
I think you did it well too complicated. You already have one instance of "logging": logger that can be used, you just need to print out in the general log cloud_correction parameters from the config file (and also you can print out MCP version if you want, but this I think we should change such that it is done for all the scripts) |
…ogger improvements, and units changes.
|
Hi @nzywucka, the latest changes seem to went into another file. |
|
Hi @nzywucka |
extracted_lidar_reports_test.csv: file with the cloud parameters from LIDAR lst1_magic_cloud_correction.py: interpolation function has been added to the code
| @@ -0,0 +1,7 @@ | |||
| timestamp,base_height,top_height,transmission,lidar_zenith | |||
There was a problem hiding this comment.
better to put this file in resources directory (where the input cards are)
There was a problem hiding this comment.
also, is this file still used for something? the yaml file has a different format
There was a problem hiding this comment.
this comment is still valid
lst1_magic_cloud_correction.py: interpolation implementation has been added to the code.
lidar_reports_clouds_ST.03.16.yaml: configuration file with LIDAR reports of ST.03.16 period added lst1_magic_cloud_correction.py: additional cleaning procedure and improvements to the interpolation function added
| @@ -0,0 +1,533 @@ | |||
| units: | |||
| timestamp: 'iso' | |||
There was a problem hiding this comment.
this file is for a very specific dataset, therefore there is no point of posting it into the general repository
There was a problem hiding this comment.
this comment is still valid
lst1_magic_cloud_correction.py: update on the additional cleaning method
| A tuple containing interpolated or nearest values for: | ||
| - base_height (float): The base height of the cloud layer in meters. | ||
| - top_height (float): The top height of the cloud layer in meters. | ||
| - vertical_transmission (float): Transmission factor of the cloud layer adjusted for vertical angle. |
|
|
||
| # AC MAGIC | ||
| # Ignore runtime warnings appeared during the image cleaning | ||
| warnings.simplefilter("ignore", category=RuntimeWarning) |
There was a problem hiding this comment.
even while this is copied from the magic_calib_do_dl1 I think it would be better to try to run without this and see if there are no issues. Because it might also hide some other warnings related to different parts of the code
| peak_time = dl1_images["peak_time"][index_img] | ||
| image /= trans_pixels | ||
|
|
||
| clean_image = image[clean_mask] |
There was a problem hiding this comment.
you use the old mask first no matter if you do the additional cleaning a moment later or not. I think it is faster to move those 3 lines as "else" after the next condition
| prefixed_conc_params = { | ||
| f"concentration_{key}": value for key, value in conc_params.items() | ||
| } | ||
| event_params.update(prefixed_conc_params) |
There was a problem hiding this comment.
prefixed_conc_params could not be included directly in event_params a few lines above?
|
|
||
| cloud_params = lidar_cloud_interpolation( | ||
| mean_subrun_timestamp, max_gap_lidar_shots, lidar_report_file | ||
| ) |
There was a problem hiding this comment.
this function can return None if there are no proper LIDAR raports to interpolate from that would crash the code below. A better way to handle error management would be to introduce a new error here:
https://github.com/cta-observatory/magic-cta-pipe/blob/73e4a3bf47ace47deb11d1d74a4caf0c37954841/magicctapipe/utils/error_codes.py
and throw it like here:
| correction_params = config.get("cloud_correction", {}) | ||
| max_gap_lidar_shots = u.Quantity( | ||
| correction_params.get("max_gap_lidar_shots") | ||
| ) # set it to 900 seconds |
There was a problem hiding this comment.
the comment is misleading - the code will set it to whatever is in the input card
| try: | ||
| dl1_images = read_table(args.input_file, image_node_path) | ||
| except tables.NoSuchNodeError: | ||
| logger.info(f"\nNo image for telescope with ID {tel_id}. Skipping.") |
There was a problem hiding this comment.
this skips the telescopes without stored images and it is very dangerous because images are stored separately in MAGIC and in LST files, so e.g. if you have LST file without images, but MAGIC with images the script will make MAGIC-only analysis that is likely not what the user wanted.
So I would turn this into a fatal error
| logger.info(f"\nNo image for telescope with ID {tel_id}. Skipping.") | ||
| dl1_images = None | ||
|
|
||
| df = process_telescope_data( |
There was a problem hiding this comment.
you pass the config to the function, so the parameters like tel_ids, nlayers, cmf should be recovered from the config inside this function instead of being passed additionally.
|
|
||
| subarray_info.to_hdf(output_file) | ||
|
|
||
| correction_params = config.get("cloud_correction", {}) |
There was a problem hiding this comment.
you already extracted it in L583, why do you do it again?
There was a problem hiding this comment.
still valid (just line changed)
| cloud_params = lidar_cloud_interpolation( | ||
| mean_subrun_timestamp, max_gap_lidar_shots, lidar_report_file | ||
| ) | ||
|
|
There was a problem hiding this comment.
also I think it would be useful to print the cloud_params in the logger
…meters in the file