Export trigger threshold as parameter - #2457
Conversation
… into export-threshold
GernotMaier
left a comment
GernotMaier
left a comment
There was a problem hiding this comment.
As discussed, remove the 1.35 (it is in quite many places) and add it as a command line parameter.
Questions (plus some minor stuff as comments):
- what happens if no intersection point is found. Does this give an error or just writes something (what)?
- are --ymin and --ymax ignored whenever data exists?
|
|
||
| _logger.info("Plotting bias curves...") | ||
| plot_tables.plot_bias_curves(nsb_stats, proton_stats, args, plot_output_path) | ||
| # Log the data points for debugging |
There was a problem hiding this comment.
Is that comment correct? It is a _logger.info statement, not a a debug statement.
There was a problem hiding this comment.
Edited the comment, thanks
| for thresh in thresholds: | ||
| nsb_rate = nsb_stats[thresh]["rate_hz"] if thresh in nsb_stats else None | ||
| proton_rate = proton_stats[thresh]["rate_hz"] if thresh in proton_stats else None | ||
| scaled_proton = 1.35 * proton_rate if proton_rate is not None else None |
There was a problem hiding this comment.
Here is the magic number! As discussed, move it to a command line parameter.
| """ | ||
| Calculate trigger threshold from bias curve intersection. | ||
|
|
||
| Trigger threshold is calculated as the intersection between NSB curve and 1.35*proton curve. |
There was a problem hiding this comment.
no magic numbers in comments.
| if not telescope_name: | ||
| _logger.warning("No telescope name provided. Using 'unknown' as telescope name.") | ||
| telescope_name = "unknown" | ||
| parameter_version = args.get("parameter_version") |
There was a problem hiding this comment.
If this is not set on the CL ('None'), does it still try to write a model data file?
There was a problem hiding this comment.
Made parameter version a required argument
There was a problem hiding this comment.
Pull request overview
Adds trigger-threshold derivation and model-parameter export to the bias-curve workflow.
Changes:
- Calculates and exports analog/digital trigger thresholds.
- Enhances plots with fitted/scaled proton curves and threshold markers.
- Expands unit coverage and explicit HDF5 reading.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/simtools/applications/derive_bias_curves.py |
Adds parameter-version CLI support. |
src/simtools/simtel/bias_curve_generator.py |
Calculates and exports trigger thresholds. |
src/simtools/simtel/nsb_trigger_calculator.py |
Explicitly reads HDF5 format. |
src/simtools/visualization/plot_tables.py |
Adds scaled curves, threshold markers, and dynamic limits. |
tests/unit_tests/simtel/test_bias_curve_generator.py |
Tests threshold calculation and export behavior. |
tests/unit_tests/visualization/test_plot_tables.py |
Tests enhanced bias-curve plotting. |
docs/changes/2457.maintenance.md |
Records the change. |
Suppressed comments (1)
tests/unit_tests/simtel/test_bias_curve_generator.py:665
- This file-writing test should use the repository's shared
tmp_test_directoryfixture rather thantmp_path(tests/unit_tests/conftest.py:240).
def test_group_hdf5_files_skips_missing_metadata(tmp_path):
"""Test that _group_hdf5_files_by_threshold_and_run skips files with missing metadata."""
# Create a file without proper metadata
(tmp_path / "proton_run000001.reduced_event_data.hdf5").touch()
result = bias_curve_generator._group_hdf5_files_by_threshold_and_run(tmp_path)
assert result == {} # File skipped due to missing threshold/run
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks for the review @GernotMaier! Made the changes requested by you and Copilot, please let me know if it's all ok now!
It raises a ValueError and no output is written.
Yes, removed ymin and ymax for this reason. |
|
GernotMaier
left a comment
GernotMaier
left a comment
There was a problem hiding this comment.
Good, thanks! I approve, but please fix the comments below (especially about the duplication of the documentation).
| # Scale proton rates by 1.35 to account for ions we didn't simulate | ||
| scaled_proton_rates = 1.35 * proton_rates | ||
| # Scale proton rates to account for ions we didn't simulate | ||
| scaling_factor = args.get("scaling_factor", 1.35) |
There was a problem hiding this comment.
Do we duplicate here the default value? That might lead later to errors.
| The input files can be generated using simtools-generate-bias-curve-submissions. | ||
| The input files can be generated using ``simtools-generate-bias-curve-submissions``. | ||
|
|
||
| **Command line arguments** |
There was a problem hiding this comment.
I think the description of the command line arguments is duplicated now: it is written here in plain text and added with the sphinx macro
## Command line arguments
```{eval-rst}
.. simtools-cli-help::
:application: derive_bias_curves
:no-heading:
Please check the html doc page and remove the 'hand written description'.
|
|
||
| .. code-block:: console | ||
|
|
||
| simtools-derive-bias-curves \\ |
There was a problem hiding this comment.
simtools-derive-bias-curves or simtools-generate-bias-curve-submissions?
Also please if possible simple list the integration tests here as example (with the corresponding sphinx macros, see the other application markdown documents).
Exporting trigger (asum or dsum) threshold as model parameter and updated plots to include fits and show trigger threshold: