Stats calc tool #2628
Stats calc tool #2628
Conversation
TjarkMiener
requested review from
maxnoe,
mexanick,
kosack,
FrancaCassol,
Hckjs and
ctoennis
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| ), | ||
| ).tag(config=True) | ||
|
|
||
| dl1a_column_name = CaselessStrEnum( |
There was a problem hiding this comment.
Is DL1a/b is "official"? Also, I'd perhaps use generic input_column_name similar to the output one.
There was a problem hiding this comment.
no, in ctapipe we use DL1_IMAGES and DL1_PARAMETERS to distinguish between things that are per-pixel vs. single quantities per event.
https://ctapipe.readthedocs.io/en/latest/api/ctapipe.io.DataLevel.html
There was a problem hiding this comment.
I'd also not make this an enum. In the generic tool, users should be able to chose any column that has compatible shape. Just provide a clear error when the column is not found in the input file.
There was a problem hiding this comment.
This could also be a list of columns, to compute on multiple at the same time.
There was a problem hiding this comment.
Ok, I changed the column name and also polish the references to DL1a data by using pixel-wise image data which is more descriptive. ToolConfigurationError is raised once the column is not found. Having list of columns seems a little bit of an overkill here, which would just make the code more complex. Maybe the aggregation config could be shared between the columns, but especially the outlier detection will be different between the columns.
There was a problem hiding this comment.
ToolConfigurationError is raised once the column is not found. Having list of columns seems a little bit of an overkill here, which would just make the code more complex. Maybe the aggregation config could be shared between the columns, but especially the outlier detection will be different between the columns.
I think the case where you only want to know about a single column is quite rare, you are usually interested in multiple. So having to read all data again to compute metrics on a new column seems very limiting and a loop over columns shouldn't make the code much more complex.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Currently fails to read prod3 files (which have no EFFECTIVE focal length information). The tool current fails with a focal_length_choice exception, however it seems there is no way to set the focal length choioce since the TableLoader is not set up to be configrable.
prod 5 files should have effective focal length |
| # Iterate over the telescope ids and calculate the statistics | ||
| for tel_id in self.tel_ids: | ||
| # Read the whole dl1 images for one particular telescope | ||
| dl1_table = self.input_data.read_telescope_events_by_id( |
There was a problem hiding this comment.
I get a crash here:
% ctapipe-calculate-pixel-statistics -i events.dl1.h5 -o stats.h5
dl1_table = self.input_data.read_telescope_events_by_id(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/kkosack/Projects/CTA/Working/ctapipe/src/ctapipe/io/tableloader.py", line 1089, in read_telescope_events_by_id
tel_ids = self.subarray.get_tel_ids(telescopes)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/kkosack/Projects/CTA/Working/ctapipe/src/ctapipe/instrument/subarray.py", line 549, in get_tel_ids
for telescope in telescopes:
TypeError: 'numpy.int16' object is not iterableSeems to be due to passing an integer instead of a list, which is what is required by read_telescope_events_by_id
| dl1_table = self.input_data.read_telescope_events_by_id( | |
| dl1_table = self.input_data.read_telescope_events_by_id( | |
| telescopes = [tel_id,] |
How does this work in the tests?
There was a problem hiding this comment.
Good point! I do not know why the test pass here. I will include the change.
In the output, how can I tell what column was aggregated? It is always named "statistics" and there is no metadata in the group or tables that contain that information. Wouldn't it be better to name the group like |
There was a problem hiding this comment.
A more general comment: with very minor changes, this could be turned into ctapipe-calculate-stats, i.e. the ability to compute stats for any column, not just pixel-wise ones.
- Expose TableLoader as a configurable component (needed anyhow, see above)
- minor modifications to drop assumption on data shape in
calculator.py.
I would expect e.g. to be able to do:
ctapipe-calculate-pixel-statistics -i events-prod5.DL1.h5
--StatisticsAggregator.chunk_size=100
--StatisticsCalculatorTool.input_column_name hillas_length
-o length.h5
and get the stats on the length parameter. This is perhaps outside the scope of this PR, but should be kept in mind. It also relates to @maxnoe's comment that we could change the API to accept a mapping of columns to Aggragators.
There was a problem hiding this comment.
A common error is to have too small a chunk size, but this now results in a very ugly error and a full trace-back and exception, along with an UnclosedFileWarning (bug?)
- The former (Unexpected exception) should e caught and raises as a
ToolConfigurationError, so the user gets a nice message. And please explain in the message what parameters controls this, i.e. sayChange --StatisticsAggregator.chunk_size to decrease this. - The latter (unclosed file) seems to be a bug to fix.
2024-11-06 15:14:43,361 ERROR [ctapipe.StatisticsCalculatorTool] (tool.run): Caught unexpected exception: The length of the provided table (853) is insufficient to meet the required statistics for a single chunk of size (2500).
2024-11-06 15:14:43,361 ERROR [ctapipe.StatisticsCalculatorTool] (tool.run): Caught unexpected exception: The length of the provided table (853) is insufficient to meet the required statistics for a single chunk of size (2500).
Traceback (most recent call last):
File "/Users/kkosack/Projects/CTA/Working/ctapipe/src/ctapipe/core/tool.py", line 431, in run
self.start()
File "/Users/kkosack/Projects/CTA/Working/ctapipe/src/ctapipe/tools/calculate_pixel_stats.py", line 134, in start
aggregated_stats = self.stats_calculator.first_pass(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/kkosack/Projects/CTA/Working/ctapipe/src/ctapipe/monitoring/calculator.py", line 169, in first_pass
aggregated_stats = aggregator(
^^^^^^^^^^^
File "/Users/kkosack/Projects/CTA/Working/ctapipe/src/ctapipe/monitoring/aggregator.py", line 86, in __call__
raise ValueError(
ValueError: The length of the provided table (853) is insufficient to meet the required statistics for a single chunk of size (2500).
2024-11-06 15:14:43,377 INFO [ctapipe.StatisticsCalculatorTool] (tool.write_provenance): Output:
/Users/kkosack/miniconda3/envs/ctapipe-0.21/lib/python3.12/site-packages/tables/file.py:113: UnclosedFileWarning: Closing remaining open file: /Users/kkosack/Projects/CTA/PipeWork/v0.21.3/events-prod5.DL1.h5
warnings.warn(UnclosedFileWarning(msg))
Since we should also support the processing of MCs, we might want to run the stats calc tool over multiple tels.
rename the tool and file name only keep dl1 table of the particular telescope into RAM added tests for tool config errors rename input col name adopt yaml syntax in example config for stats calculation
renamed output_column_name to output_table_name
TjarkMiener
force-pushed
the
stats_calc_tool
branch
from
07587ad to
b0c197c
Compare
This comment has been minimized.
This comment has been minimized.
remove the input url since it is irrelevant
This comment has been minimized.
This comment has been minimized.
mexanick
dismissed
kosack’s stale review
Code updated, please take a second look.
| Perform statistics calculation for pixel-wise image data | ||
| """ | ||
|
|
||
| name = "StatisticsCalculatorTool" |
There was a problem hiding this comment.
| name = "StatisticsCalculatorTool" | |
| name = "ctapipe-calculate-pixel-statistics" |
(should be the name of the command-line tool, not the class, which already has a name)
There was a problem hiding this comment.
Also should the class be PixelStatisticsCalculatorTool? Or do you intend to eventually extend it to stats on non-pixel-wise quantities?
There was a problem hiding this comment.
The suggested renaming makes sense since the tool and the used components are tailored to pixel-wise quantities. First, we planned to re-use the components for any monitoring data. However, for most of the monitoring data, the astropy aggregation should be sufficient.
Analysis Details0 Issues
Coverage and Duplications
Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs |
This PR adds a generic stats-calculation tool utilizing the
PixelStatisticsCalculator.Related #2542