More ImPACT code cleanup#2705
Conversation
| filename, role="ImPACT Time Template file for " + tel_type[t] | ||
| for template_path, tel_ids in template_sort_dict.items(): | ||
| net_interpolator = TemplateNetworkInterpolator( | ||
| template_path, bounds=((-5, 1), (-1.5, 1.5)) |
There was a problem hiding this comment.
Is it really wise to hardcode the bounds of the templates this deep into a function? I think you should at least extract these definitions to some global constants at the top of the file.
There was a problem hiding this comment.
Yes putting them as a global constant is better than this
Analysis Details7 Issues
Coverage and Duplications
Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs |
|
Hi all, are there any actions that are needed to merge this PR? I think these commits are an important step to prepare for a FreePACT PR. |
|
@kosack @ParsonsRD the static analysis was failing with lots of duplicated code and too little coverage, so I didn't yet have a deeper look. 30 % duplications is quite a lot... The branch needs an update with main also. |
| impact_reco = ImPACTReconstructor(example_subarray, table_profile) | ||
| for tel_type in example_subarray.telescope_types: | ||
| create_dummy_templates( | ||
| str(tmp_path) + "/{}.template.gz".format(str(tel_type)), |
There was a problem hiding this comment.
| str(tmp_path) + "/{}.template.gz".format(str(tel_type)), | |
| tmp_path / f"{tel_type}.template.gz", |
| [ | ||
| "type", | ||
| "MST_MST_FlashCam", | ||
| str(tmp_path) + "/MST_MST_FlashCam.template.gz", |
There was a problem hiding this comment.
| str(tmp_path) + "/MST_MST_FlashCam.template.gz", | |
| tmp_path / "MST_MST_FlashCam.template.gz", |
| spe = 0.6 # Also hard code single p.e. distribution width | ||
| # The SPE and pedestal width parameters are also configurable as TelescopeParameters. | ||
| # None is allowed and the default value. In that case, either values from the event monitoring data are used | ||
| # or if that is also not available, a value from a hardcoded backup dict is used. |
There was a problem hiding this comment.
I think that is a bad idea, since it means if somebody forgets to set a correct value, the backup value is silently used, and it is not clear to the user that that happened. It would be better to have it fail if a telescope type is not specified, especially since the backup values are wrong for Prod6. The backup values don't even end up in the provenance log, since they are set outside the configuration system.
I would just remove BACKUP_PED_TABLE and BACKUP_SPE_TABLE, and instead add a sample configuration file to the resources directory and update ctapipe-quickstart to export it (or modify the existing example file) if you really want to have some defaults. However, even there it's not such a good idea since they are really only correct for one PROD.
Eventually (but perhaps in a future PR if it's too complex), these should just be read from the interpolated monitoring data produced by the MonitoringSource, which you can access via the event itself: that's possible already for the pedestal width (after ctapipe-v0.27, see changelog), but I think not yet for the PE/DC ratio. With that, you don't even have to specify a fixed pedestal variance or PE/DC ratio, since that allows it to vary during an observation. That's much safer, since the user doesn't have to copy this information into the config file.
| mask = event.dl1.tel[tel_id].image_mask | ||
| if self.pedestal_width.tel[tel_id] is None: | ||
| if event.mon.tel[tel_id].pedestal.charge_std is None: | ||
| ped_dict[tel_id] = BACKUP_PED_TABLE[ |
There was a problem hiding this comment.
I would just remove this fallback (see prev comment), and just require that the user use a proper configuration file.
|
Also, please rebase this on the main branch, since the reason the tests are not all passing is just that some things were changed in the test configuration (like the SonarQube server) since this MR was opened. |
5 participants
Hi everyone,
I finally got around to clean up the ImPACT code a bit more.
This PR has 3 major changes:
I look forward to your comments and to discussion on the implementation of these things.