Save pulse shape parameters directly in json (and not in a table file) by GernotMaier · Pull Request #2088 · gammasim/simtools

GernotMaier · 2026-03-26T11:57:18Z

This is a larger PR to allow the storage of tabled date directly in the model parameter json and not in externally linked files. Implemented here for the fadc_pulse_shape parameter but will be extended to all tabled data (and some items working for for fadc_pulse_shape will be generalized.

This pull request introduces a unified and more robust export flow for model parameter tables, improves schema selection and validation, and enhances documentation and CLI usability. The changes ensure stricter validation and normalization of table content, support for schema versioning, and clearer export logic for both file-backed and dict-backed parameters. The documentation and command-line interfaces are updated to reflect these improvements and provide better guidance to users.

Parameter export and validation improvements:

Implemented a unified export flow for both file-backed and dict-backed table parameters, with stricter validation and normalization (including required column_units for fadc_pulse_shape). The export logic now ensures consistent handling and output formats for all table-type parameters. [1] [2] [3]
Added support for selecting the schema document by model_parameter_schema_version (with fallback to the latest version), and exposed schema version selection in CLI and application logic. [1] [2] [3] [4] [5]

Database and model parameter handling:

Refactored the database handler to delegate export logic to a new parameter_exporter module, aligning sim_telarray/model/database handling and expanding unit test coverage. [1] [2] [3]
Improved error handling and logging in database file export applications, raising clear exceptions and providing informative log messages. [1] [2]

Documentation and CLI usability:

Updated CLI documentation and argument parsing for parameter export applications to clarify output modes, file naming, and schema versioning. Added detailed usage examples for all export scenarios. [1] [2] [3] [4] [5] [6]
Added API documentation entries for the new parameter_exporter and simtel_table_writer modules. [1] [2]

Model data writer enhancements:

Added a method to retrieve parameter types from a selected schema version, and ensured metadata is written only when present during parameter submission. [1] [2] [3]

Typical applications / steps:

Submit an existing simtel_array style table and store it in the new dict-backed table parameter:

simtools-submit-model-parameter-from-external --parameter fadc_pulse_shape --value LST-1_spe_pulse_shape_pix3.dat --instrument LSTN-01 --parameter_version 4.0.0 --data_path ../simulation-model-parameter-setting/input/LSTN-01/fadc_pulse_shape/20260316T110922

(Requires to have the the corresponding simulation model setting branch checked out)

Getting the new dict-type parameter as astropy table

python src/simtools/applications/db_get_parameter_from_db.py --parameter fadc_pulse_shape --parameter_version 2.0.0 --telescope LSTN-01 --site North --export_model_file  --export_model_file_as_table --output_file fadc_pulse_shape.ecsv\

Testing this branch
Requires to run with a test branch of simulation models: fadc-pulse-shape-format-change

src/simtools/simtel/simtel_table_reader.py

Pulse shape as value b

Copilot

Pull request overview

Enable storing table-like model parameter values directly inside model-parameter JSON (initially for fadc_pulse_shape), while unifying export/validation logic across file-backed and dict-backed parameters and selecting schema entries by model_parameter_schema_version.

Changes:

Add row-oriented table serialization + table-file writer/reader utilities and wire them into sim_telarray config generation and validation flows.
Introduce parameter_exporter module and delegate DB export logic to it (including dict-backed → ECSV export support).
Update schemas, CLI apps, docs, reference sim_telarray configs, and unit tests for schema-version selection and dict-backed table handling.

Reviewed changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/unit_tests/visualization/test_plot_tables.py	Update plotting tests for schema-version selection and parameter-dict lookup.
tests/unit_tests/simtel/test_simulator_light_emission.py	Update patches to use new `simtel_table_writer` functions.
tests/unit_tests/simtel/test_simtel_table_writer.py	Add unit tests for the new sim_telarray table writer.
tests/unit_tests/simtel/test_simtel_table_reader.py	Add tests for row-data serialization and dict-value resolution helpers.
tests/unit_tests/simtel/test_simtel_output_validator.py	Add tests ensuring dict-typed metadata values can be resolved from table files.
tests/unit_tests/simtel/test_simtel_config_writer.py	Update tests to use new writer functions; add tests for new conversion helpers.
tests/unit_tests/model/test_model_parameter.py	Add coverage for legacy table-value resolution during schema upgrades.
tests/unit_tests/model/test_legacy_model_parameter.py	Add/extend legacy migration handlers for `fadc_pulse_shape` and resolver plumbing.
tests/unit_tests/db/test_parameter_exporter.py	Add unit tests for the new DB parameter export orchestration module.
tests/unit_tests/db/test_db_handler.py	Update DB handler tests for delegation and dict-backed export behavior.
tests/unit_tests/data_model/test_model_data_writer.py	Extend writer tests for embedded `fadc_pulse_shape` and schema-version type lookup.
tests/resources/sim_telarray_configurations/6.0.2/CTA-South-SSTS-01_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/6.0.2/CTA-South-MSTS-01_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/6.0.2/CTA-South-LSTS-01_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/6.0.2/CTA-North-MSTN-01_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/6.0.2/CTA-North-LSTN-02_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/6.0.2/CTA-North-LSTN-01_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/5.0.0/CTA-South-SSTS-01_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/5.0.0/CTA-South-MSTS-01_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/5.0.0/CTA-South-LSTS-01_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/5.0.0/CTA-North-MSTN-01_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/5.0.0/CTA-North-LSTN-02_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
tests/resources/sim_telarray_configurations/5.0.0/CTA-North-LSTN-01_test.cfg	Update expected `fadc_pulse_shape` filename and config_release.
src/simtools/visualization/plot_tables.py	Select schema entry by parameter’s schema version before generating plot configs.
src/simtools/simtel/simulator_light_emission.py	Switch pulse/angular-distribution table writing to `simtel_table_writer`.
src/simtools/simtel/simtel_table_writer.py	New module for writing sim_telarray table files (pulse/angle/generic tables).
src/simtools/simtel/simtel_table_reader.py	Add row-data serialization, unit normalization, and dict-value resolution helpers.
src/simtools/simtel/simtel_output_validator.py	Resolve dict-typed metadata values from table files prior to comparison.
src/simtools/simtel/simtel_config_writer.py	Remove embedded table-writer methods; write dict table parameters via new writer.
src/simtools/schemas/model_parameters/fadc_pulse_shape.schema.yml	Add schema v0.2.0 for embedded row-data representation and plot configuration.
src/simtools/model/model_parameter.py	Pass legacy value resolver into schema-upgrade path and implement table-value resolver.
src/simtools/model/legacy_model_parameter.py	Add value_resolver plumbing + fadc_pulse_shape migration to embedded row-data.
src/simtools/io/ascii_handler.py	Update JSON writing and add compact encoding for numeric lists (row tables).
src/simtools/db/parameter_exporter.py	New shared export logic for file-backed and dict-backed parameter payloads.
src/simtools/db/db_handler.py	Delegate export operations to `parameter_exporter`; add `export_parameter_data`.
src/simtools/data_model/model_data_writer.py	Write metadata only after successful validation; add schema-version type lookup.
src/simtools/applications/submit_model_parameter_from_external.py	Add schema-version arg and pre-resolve dict parameter values for submission.
src/simtools/applications/db_get_parameter_from_db.py	Rework CLI export modes; use `export_parameter_data` for unified exports.
src/simtools/applications/db_get_file_from_db.py	Improve error handling by raising a clear FileNotFoundError with context.
docs/source/api-reference/sim_telarray.md	Add API docs entry for `simtel_table_writer`.
docs/source/api-reference/db_handler.md	Add API docs entry for `db.parameter_exporter`.
docs/changes/2088.feature.md	Add changelog entry describing unified export flow and schema selection changes.

src/simtools/simtel/simtel_table_writer.py

src/simtools/schemas/model_parameters/fadc_pulse_shape.schema.yml

src/simtools/applications/submit_model_parameter_from_external.py

- Move astropy.units import to top of row_table_utils.py to fix pylint c0415 - Move row_table_utils import to top of model_data_writer.py to fix pylint c0415 - All pre-commit hooks now pass - All 806 tests continue to pass

Copilot

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated 3 comments.

docs/source/api-reference/sim_telarray.md

tests/unit_tests/conftest.py

tests/unit_tests/db/test_parameter_exporter.py

EshitaJoshi

Thanks for the PR! I have minor comments, but I'm approving already as on the whole it all looks fine to me. But some comments are really minor and you can decide if you want to ignore them.

EshitaJoshi · 2026-04-08T10:24:24Z

src/simtools/data_model/row_table_utils.py

+    columns = value["columns"]
+    rows = value["rows"]
+
+    if require_column_units:


But the previous functions is_row_table_schema and is_row_table_dict both require a units column otherwise they return False, so when would we ever pass require_column_units=False to this function?

This is needed for the case of writing in legacy sim_telarray format (function is called from simtel_table_writer:write_simtel_table with require_column_units=False)

EshitaJoshi · 2026-04-08T10:31:12Z

src/simtools/data_model/row_table_utils.py

+    columns = value["columns"]
+    rows = value["rows"]


Missing validation for columns and rows here. They need to be (list, tuple). Also good to check that column names are strings.

EshitaJoshi · 2026-04-08T11:13:13Z

src/simtools/db/parameter_exporter.py

+        return [
+            info["value"]
+            for info in parameters.values()
+            if info and info.get("file") and info["value"] is not None


Just for consistency maybe do if isinstance(info, dict) and info.get("file") and info.get("value") is not None

EshitaJoshi · 2026-04-08T11:29:54Z

src/simtools/io/ascii_handler.py

+
+def _is_numeric_list(obj):
+    """Return True if obj is a list whose elements are all int or float."""
+    return isinstance(obj, list) and bool(obj) and all(isinstance(v, (int, float)) for v in obj)


In the return statement and obj will evaluate the same as and bool(obj)

EshitaJoshi · 2026-04-08T14:04:18Z

src/simtools/db/parameter_exporter.py

+
+def _is_dict_table_value(parameter_info):
+    """Return True if a parameter stores embedded row-oriented table data."""
+    return parameter_info.get("type") == "dict" and isinstance(parameter_info.get("value"), dict)


I think checking if value is also a dict is not enough here because not all dictionaries are tables. I think you need to check is_row_table_dict(value) here, because otherwise later row_data_to_astropy_table will fail if value doesn't have 'columns' and 'rows' as keys.

GernotMaier · 2026-04-09T10:30:11Z

Thanks a lot @EshitaJoshi ! I've addressed all your comments - wait now for the test and will merge then.

ctao-sonarqube · 2026-04-09T10:34:18Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
95.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

Save pulse shape parameters directly in json (and not in a table file)

a932a6e

GernotMaier self-assigned this Mar 26, 2026

github-code-quality bot found potential problems Mar 26, 2026

View reviewed changes

src/simtools/simtel/simtel_table_reader.py Fixed Show fixed Hide fixed

GernotMaier added 27 commits March 26, 2026 14:40

sonar

234f1c5

Merge branch 'pulse-shape-as-value'

06be2b9

Merge branch 'main' into pulse-shape-as-value

6a14053

table as row data

4648399

export as astropy table

1680282

export as astropy table

9bbc1e0

improved module exporter

3cd8b90

test fix

526fa75

Merge branch 'pulse-shape-as-value' into pulse-shape-as-value-b

e149130

Merge pull request #2090 from gammasim/pulse-shape-as-value-b

07ed831

Pulse shape as value b

docs

e397648

update parameter

6d436cc

update parameter

17c6b45

update parameter

ce81dd2

unit tests

30c80fd

table writer

2f690ca

table writer

bc376f2

docstrings [skip ci]

02b9fb0

simplification [skip ci]

481f577

Merge branch 'main' into pulse-shape-as-value

5cb22ea

improved docstrings

db9cd73

add column units

7eff479

Merge branch 'main' into pulse-shape-as-value

28111b5

Merge branch 'git-safe-dir-simtools' into pulse-shape-as-value

bcddc87

Merge branch 'main' into pulse-shape-as-value

80ff08f

plot table with schema

9e54b37

tmp test directory

f534db4

GernotMaier added 3 commits March 30, 2026 16:42

remove duplicated import

52a6527

cfg for tests update

5529417

Merge branch 'main' into pulse-shape-as-value

a801f67

GernotMaier marked this pull request as ready for review March 30, 2026 17:30

GernotMaier requested a review from Copilot March 30, 2026 17:30

Copilot started reviewing on behalf of GernotMaier March 30, 2026 17:31 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

src/simtools/simtel/simtel_table_writer.py Show resolved Hide resolved

src/simtools/schemas/model_parameters/fadc_pulse_shape.schema.yml Outdated Show resolved Hide resolved

src/simtools/applications/submit_model_parameter_from_external.py Show resolved Hide resolved

GernotMaier added 8 commits March 30, 2026 20:07

dict test

a37121e

consolitation

f3a58cc

accidental commit

0e4a18e

accidental commit

4bb5824

new module

e6a20ba

fix db conflicts

4036d99

minor renaming of row table utils module

7c67f9e

GernotMaier requested a review from Copilot March 31, 2026 09:26

Copilot started reviewing on behalf of GernotMaier March 31, 2026 09:27 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

docs/source/api-reference/sim_telarray.md Outdated Show resolved Hide resolved

tests/unit_tests/conftest.py Show resolved Hide resolved

tests/unit_tests/db/test_parameter_exporter.py Show resolved Hide resolved

GernotMaier added 2 commits March 31, 2026 11:35

Merge branch 'main' into pulse-shape-as-value

e433c38

cp review

5d5097b

GernotMaier requested a review from EshitaJoshi March 31, 2026 14:23

Merge branch 'main' into pulse-shape-as-value

186f43a

EshitaJoshi approved these changes Apr 8, 2026

View reviewed changes

GernotMaier added 3 commits April 9, 2026 11:54

Merge branch 'main' into pulse-shape-as-value

547c457

improvements reviewer comments

9855074

Merge branch 'main' into pulse-shape-as-value

da8442a

GernotMaier merged commit 1044744 into main Apr 9, 2026
22 of 23 checks passed

GernotMaier deleted the pulse-shape-as-value branch April 9, 2026 10:52

Conversation

GernotMaier commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Typical applications / steps:

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EshitaJoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GernotMaier commented Apr 9, 2026

Uh oh!

ctao-sonarqube bot commented Apr 9, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GernotMaier commented Mar 26, 2026 •

edited

Loading