Model data writer - readability and dict-table output#2121
Merged
Conversation
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors ModelDataWriter to clarify its public API and ensures consistent JSON serialization for model-parameter files (notably “dict-table” style parameters), including when parameters are pulled from settings-workflow repositories.
Changes:
- Rename and separate writer entry points (e.g.,
dump*→write_*,write()→write_data()), and update call sites accordingly. - Centralize JSON writing options via
ModelDataWriter.write_model_parameter_json()to improve dict-table readability/consistency. - Update unit tests to reflect the new API and patch points.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/ray_tracing/test_psf_parameter_optimisation.py | Update mocks/assertions to use write_model_parameter. |
| tests/unit_tests/ray_tracing/test_mirror_panel_psf.py | Patch write_model_parameter instead of dump_model_parameter. |
| tests/unit_tests/ray_tracing/test_incident_angles.py | Update expectations for new static writer methods. |
| tests/unit_tests/model/test_model_repository.py | Patch write_model_parameter_json/write_model_parameter and adjust assertions. |
| tests/unit_tests/layout/test_array_layout_utils.py | Update expected call from dump_model_parameter to write_model_parameter. |
| tests/unit_tests/data_model/test_model_data_writer.py | Align tests with renamed writer API and JSON-writing helpers. |
| tests/unit_tests/camera/test_single_photon_electron_spectrum.py | Patch write_product_data instead of dump. |
| tests/unit_tests/camera/test_camera_efficiency.py | Patch write_model_parameter instead of dump_model_parameter. |
| src/simtools/ray_tracing/psf_parameter_optimisation.py | Use write_model_parameter for PSF parameter export. |
| src/simtools/ray_tracing/mirror_panel_psf.py | Use write_model_parameter for RNDA export. |
| src/simtools/ray_tracing/incident_angles.py | Switch to static write_product_data + write_model_parameter. |
| src/simtools/model/model_repository.py | Use centralized write_model_parameter_json for workflow-downloaded parameters and rename writer call. |
| src/simtools/layout/array_layout_utils.py | Switch to write_model_parameter for layout/element parameter writes. |
| src/simtools/data_model/model_data_writer.py | Refactor/rename API and introduce write_model_parameter_json. |
| src/simtools/camera/single_photon_electron_spectrum.py | Use write_product_data for output writing. |
| src/simtools/camera/camera_efficiency.py | Use write_model_parameter for NSB pixel rate parameter output. |
| src/simtools/applications/submit_model_parameter_from_external.py | Switch to write_model_parameter API. |
| src/simtools/applications/submit_data_from_external.py | Switch to write_product_data API. |
| src/simtools/applications/generate_regular_arrays.py | Use write_product_data and adjust downstream output path for the info YAML. |
| src/simtools/applications/derive_pulse_shape_parameters.py | Use write_model_parameter API for derived parameters. |
| src/simtools/applications/db_get_array_layouts_from_db.py | Switch to write_product_data API. |
| src/simtools/applications/convert_model_parameter_from_simtel.py | Switch to write_model_parameter API. |
| src/simtools/applications/convert_geo_coordinates_of_array_elements.py | Switch to write_product_data API. |
| src/simtools/applications/convert_all_model_parameters_from_simtel.py | Switch to write_model_parameter API. |
| docs/changes/2121.maintenance.md | Add changelog fragment for the writer refactor + dict-table fix. |
tobiaskleiner
approved these changes
Apr 17, 2026
Collaborator
tobiaskleiner
left a comment
tobiaskleiner
left a comment
There was a problem hiding this comment.
Thanks @GernotMaier, good improvement, can be merged.
Contributor
Author
|
Thanks @tobiaskleiner |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two issues are addressed:
Functional: model parameters in dict-table files are not written in a readable table-like format when the parameter dict is pulled from settings workflows. Make sure now that the same write function is used in all cases.
Readability: model_data_writer had a very confusing API and it is hard to see the differences between the public functions (I buy you an icecream if you can explain the differences without checking the code / doc strings):
- dump()
- dump_model_parameter()
- write()
- write_model_parameter()
- write_dict_to_mode_parameter()
I’ve tried to separate public / private functions and improved the naming.