Overwrite parameters in model utils

[orelgueta]

This PR is just a working example for overwriting parameters also when using model_utils to setup a model, in particular in the context of validate optics. It is clearly not complete, but I needed these changes to get it to work. Adding a couple of tests should be enough I think.

In addition to that change, also the propagation of the model version into the plot of incidence angle snuck in to this PR. That is not a necessity, it was needed just to make the plot comparison easier in a different study. If you don't find it useful, please feel free to remove it.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[GernotMaier]

Following updates:

• added overwrite_model_parameters to all calls of initialize_simulation_models
• fixed unit tests
• added an integration test: scale camera_transmission parameter for validate_camera_efficiency application by a factor of 10
• added a 'scaling' factor in integration tests to allow to compare against a reference file (meaning for this example scaling=10

[GernotMaier]

Note that this type of repetitive changes is what we want do avoid in future using the general settings configuration. I will implement this in a follow-up PR - should remove the many passed on parameters.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

src/simtools/model/model_utils.py:40

• The docstring for initialize_simulation_models is missing documentation for the new overwrite_model_parameters parameter. Please add a description of this parameter to the Parameters section of the docstring, including its type and purpose.

def initialize_simulation_models(
    label,
    model_version,
    site,
    telescope_name,
    calibration_device_name=None,
    overwrite_model_parameters=None,
):
    """
    Initialize simulation models for a single telescope, site, and calibration device model.

    Parameters
    ----------
    label: str
        Label for the simulation.
    model_version: str
        Version of the simulation model
    site: str
        Name of the site.
    telescope_name: str
        Name of the telescope.
    calibration_device_name: str, optional
        Name of the calibration device.

    Returns
    -------
    Tuple
        Tuple containing the telescope site, (optional) calibration device model.
    """

src/simtools/applications/validate_cumulative_psf.py:140

• This call to overwrite_parameters_from_file appears to be redundant. The ModelParameter base class (which TelescopeModel inherits from) automatically calls overwrite_parameters_from_file in _load_parameters_from_db if overwrite_model_parameters is provided during initialization. Since tel_model is initialized with overwrite_model_parameters at line 136, the parameters should already be overwritten.

    if app_context.args.get("overwrite_model_parameters"):
        tel_model.overwrite_parameters_from_file(app_context.args["overwrite_model_parameters"])

[GernotMaier]

Not worried about the missing coverage in the plotting routines.

[GernotMaier]

@orelgueta - I cannot add you as a reviewer (as you opened the PR) - please have a look.

[orelgueta]

Thanks for implementing these changes!

I cannot approve (for the same reason I cannot officially review), but even if you do not make any changes based on the comments I made, I think this is ready to be merged.

[orelgueta]

That is quite confusing that the scaling here is the inverse of the change applied. Is it only in the case of this specific transmission example?

[GernotMaier]

This depends on the physics case and it is quite logical: this is the reference file section and for a valid comparison we need to scale the reference file by a factor of 10.

Note that there might be test cases where there is no linear relation ship as here.

Unless you think differently, I would prefer to leave this.

[orelgueta]

You can leave it of course. In the end it is not a user feature but more of a developer testing one, so it can be a little confusing.

[orelgueta]

Is it enough to test just this application and not all the rest?

[GernotMaier]

All?!?!

We will use this feature a lot in future (I already do for the corsika histograms), so I think more tests will come.

[orelgueta]

Well, I would suggest adding more cases slowly in the future, yes. Testing just one application is the reason we needed this PR in the first place. Either way, it can wait for the future.

[GernotMaier]

Thanks a lot @orelgueta - let me know if the scaling factor is ok.

Note that I am update the overwrite feature right now. This implementation is very inefficient when applying to many parameters / many models, as the yml file with the parameter is read for each parameter (and validate each time - meaning the validation schema file is also read each time!). This will come in a future PR.

[orelgueta]

All good, you can merge once the tests are fixed.

[ctao-sonarqube]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube