From 5e290c8116108ca57b3906f34ead0f7310d2a790 Mon Sep 17 00:00:00 2001 From: Gernot Maier Date: Fri, 19 Dec 2025 16:56:15 +0100 Subject: [PATCH 1/7] Unused global variable --- src/simtools/data_model/metadata_model.py | 4 ---- src/simtools/visualization/plot_corsika_histograms.py | 3 --- 2 files changed, 7 deletions(-) diff --git a/src/simtools/data_model/metadata_model.py b/src/simtools/data_model/metadata_model.py index ad03858661..2e9ac0bc1c 100644 --- a/src/simtools/data_model/metadata_model.py +++ b/src/simtools/data_model/metadata_model.py @@ -8,13 +8,9 @@ """ -import logging - import simtools.data_model.schema import simtools.utils.general as gen -_logger = logging.getLogger(__name__) - def get_default_metadata_dict( schema_file=None, observatory="CTA", schema_version="latest", lower_case=True diff --git a/src/simtools/visualization/plot_corsika_histograms.py b/src/simtools/visualization/plot_corsika_histograms.py index 43e5f6ae06..573c2c1c93 100644 --- a/src/simtools/visualization/plot_corsika_histograms.py +++ b/src/simtools/visualization/plot_corsika_histograms.py @@ -1,6 +1,5 @@ """Visualize Cherenkov photon distributions from CORSIKA.""" -import logging from pathlib import Path import matplotlib.pyplot as plt @@ -10,8 +9,6 @@ from simtools.visualization.visualize import save_figures_to_single_document -_logger = logging.getLogger(__name__) - def _plot_2d(hist_list, labels=None): """ From d053cb9cf68f208b5cab64672ee538fd10862418 Mon Sep 17 00:00:00 2001 From: Gernot Maier Date: Fri, 19 Dec 2025 16:58:49 +0100 Subject: [PATCH 2/7] Use of the return value of a procedure --- src/simtools/io/ascii_handler.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/simtools/io/ascii_handler.py b/src/simtools/io/ascii_handler.py index 6f1ce2de18..38c5fe96fc 100644 --- a/src/simtools/io/ascii_handler.py +++ b/src/simtools/io/ascii_handler.py @@ -208,9 +208,11 @@ def write_data_to_file(data, output_file, sort_keys=False, numpy_types=False): """ output_file = Path(output_file) if output_file.suffix.lower() == ".json": - return _write_to_json(data, output_file, sort_keys, numpy_types) + _write_to_json(data, output_file, sort_keys, numpy_types) + return if output_file.suffix.lower() in [".yml", ".yaml"]: - return _write_to_yaml(data, output_file, sort_keys) + _write_to_yaml(data, output_file, sort_keys) + return raise ValueError( f"Unsupported file type {output_file.suffix}. Only .json, .yml, and .yaml are supported." From 96bc1669711406e7eb18e33f8949aeddacc263aa Mon Sep 17 00:00:00 2001 From: Gernot Maier Date: Fri, 19 Dec 2025 17:00:57 +0100 Subject: [PATCH 3/7] Unused local variable --- src/simtools/configuration/commandline_parser.py | 2 +- tests/unit_tests/data_model/test_metadata_collector.py | 2 +- tests/unit_tests/layout/test_array_layout.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/simtools/configuration/commandline_parser.py b/src/simtools/configuration/commandline_parser.py index 3858ec005d..350130f6f1 100644 --- a/src/simtools/configuration/commandline_parser.py +++ b/src/simtools/configuration/commandline_parser.py @@ -269,7 +269,7 @@ def initialize_simulation_model_arguments(self, model_options): type=self.telescope, ) if "layout" in model_options or "layout_file" in model_options: - _job_group = self._add_model_option_layout( + self._add_model_option_layout( job_group=_job_group, model_options=model_options, # layout info is always required for layout related tasks with the exception diff --git a/tests/unit_tests/data_model/test_metadata_collector.py b/tests/unit_tests/data_model/test_metadata_collector.py index da86ba6c70..fa9f3b66e7 100644 --- a/tests/unit_tests/data_model/test_metadata_collector.py +++ b/tests/unit_tests/data_model/test_metadata_collector.py @@ -296,7 +296,7 @@ def test_fill_context_sim_list(args_dict_site): # one entry with Nones only _test_def = [{"site": None, "class": None, "type": None, "subtype": None, "id:": None}] - _test_def = _collector._fill_context_sim_list(_test_def, _new_element) + _collector._fill_context_sim_list(_test_def, _new_element) assert _test_none == [_new_element] diff --git a/tests/unit_tests/layout/test_array_layout.py b/tests/unit_tests/layout/test_array_layout.py index 0ff25491f3..bf68a65fd3 100644 --- a/tests/unit_tests/layout/test_array_layout.py +++ b/tests/unit_tests/layout/test_array_layout.py @@ -428,7 +428,7 @@ def test_export_telescope_list_table( layout_utm._telescope_list = [] try: - table_utm = layout_utm.export_telescope_list_table(crs_name="utm") + layout_utm.export_telescope_list_table(crs_name="utm") except IndexError: pytest.fail("IndexError raised") From 7b905005e56c0eabd283f416cbf1fd144fcc0631 Mon Sep 17 00:00:00 2001 From: Gernot Maier Date: Fri, 19 Dec 2025 17:03:40 +0100 Subject: [PATCH 4/7] An assert statement has a side-effect --- tests/unit_tests/corsika/test_corsika_config.py | 3 ++- tests/unit_tests/data_model/test_model_data_writer.py | 3 ++- tests/unit_tests/simtel/test_simulator_camera_efficiency.py | 6 ++++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/unit_tests/corsika/test_corsika_config.py b/tests/unit_tests/corsika/test_corsika_config.py index 6a5a037654..988b7ad303 100644 --- a/tests/unit_tests/corsika/test_corsika_config.py +++ b/tests/unit_tests/corsika/test_corsika_config.py @@ -557,7 +557,8 @@ def test_write_seeds_use_test_seeds(corsika_config_mock_array_model): expected_calls = [_call.args[0] for _call in mock_file.write.call_args_list] expected_seeds = [534, 220, 1104, 382] for _call in expected_calls: - assert _call == (f"SEED {expected_seeds.pop(0)} 0 0\n") + seed = expected_seeds.pop(0) + assert _call == f"SEED {seed} 0 0\n" def test_get_corsika_telescope_list(corsika_config_mock_array_model): diff --git a/tests/unit_tests/data_model/test_model_data_writer.py b/tests/unit_tests/data_model/test_model_data_writer.py index 7abf67f605..5ceba744bf 100644 --- a/tests/unit_tests/data_model/test_model_data_writer.py +++ b/tests/unit_tests/data_model/test_model_data_writer.py @@ -36,7 +36,8 @@ def num_gains_schema(num_gains_schema_file): def test_write(tmp_test_directory, args_dict_site): # both none (no exception expected) w_1 = writer.ModelDataWriter(output_path=tmp_test_directory) - assert w_1.write(metadata=None, product_data=None) is None + result = w_1.write(metadata=None, product_data=None) + assert result is None # metadata not none; no data and metadata file _metadata = metadata_collector.MetadataCollector(args_dict=args_dict_site) diff --git a/tests/unit_tests/simtel/test_simulator_camera_efficiency.py b/tests/unit_tests/simtel/test_simulator_camera_efficiency.py index df9eb5fe4f..e36b1fbdf7 100644 --- a/tests/unit_tests/simtel/test_simulator_camera_efficiency.py +++ b/tests/unit_tests/simtel/test_simulator_camera_efficiency.py @@ -148,8 +148,10 @@ def produced_file_has_expected_values(file): if line.startswith("#"): continue entry = line.split() - assert float(entry[0]) == pytest.approx(wavelengths.pop(0)) - assert float(entry[2]) == pytest.approx(nsbs.pop(0)) + expected_wavelength = wavelengths.pop(0) + expected_nsb = nsbs.pop(0) + assert float(entry[0]) == pytest.approx(expected_wavelength) + assert float(entry[2]) == pytest.approx(expected_nsb) assert len(entry) == 3 if len(wavelengths) == 0: break From b738b3fa82432eac985bf0e105c8c0f95c9d0136 Mon Sep 17 00:00:00 2001 From: Gernot Maier Date: Fri, 19 Dec 2025 17:07:33 +0100 Subject: [PATCH 5/7] Variable defined multiple times --- docs/source/conf.py | 3 --- src/simtools/ray_tracing/psf_analysis.py | 1 - tests/unit_tests/layout/test_telescope_position.py | 4 ++-- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/docs/source/conf.py b/docs/source/conf.py index 26dd2cfd1a..ca00f9094a 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -50,9 +50,6 @@ def get_python_version_from_pyproject(): project = "simtools" copyright = "2024-2025, gammasim-tools, simtools developers" # noqa A001 author = get_authors_from_citation_file() -rst_epilog = f""" -.. |author| replace:: {author} -""" python_min_requires, python_requires = get_python_version_from_pyproject() rst_epilog = f""" diff --git a/src/simtools/ray_tracing/psf_analysis.py b/src/simtools/ray_tracing/psf_analysis.py index 9fd6f5522b..3a5ced2848 100644 --- a/src/simtools/ray_tracing/psf_analysis.py +++ b/src/simtools/ray_tracing/psf_analysis.py @@ -384,7 +384,6 @@ def scan(dr, rad_min, rad_max): if radius is not found (found_radius is False) """ r0, r1 = rad_min, rad_min + dr - s0, s1 = 0, 0 found_radius = False while not found_radius: s0, s1 = self._sum_photons_in_radius(r0), self._sum_photons_in_radius(r1) diff --git a/tests/unit_tests/layout/test_telescope_position.py b/tests/unit_tests/layout/test_telescope_position.py index 063e254d39..74d7bb4ccf 100644 --- a/tests/unit_tests/layout/test_telescope_position.py +++ b/tests/unit_tests/layout/test_telescope_position.py @@ -171,9 +171,9 @@ def test_convert(crs_wgs84, crs_local, crs_utm): # errors with pytest.raises(pyproj.exceptions.CRSError): - _lat, _lon = tel._convert("crs_local", crs_wgs84, 0.0, 0.0) + tel._convert("crs_local", crs_wgs84, 0.0, 0.0) with pytest.raises(pyproj.exceptions.CRSError): - _lat, _lon = tel._convert(None, None, 0.0, 0.0) + tel._convert(None, None, 0.0, 0.0) _lat, _lon = tel._convert(crs_local, crs_wgs84, test_position["pos_x"], None) assert np.isnan(_lat) From 6b98d56c9fa668f4c51afe73cc68726ed2782150 Mon Sep 17 00:00:00 2001 From: Gernot Maier Date: Fri, 19 Dec 2025 17:21:39 +0100 Subject: [PATCH 6/7] Module is imported with 'import' and 'import from' --- .../data_model/test_metadata_collector.py | 3 +- tests/unit_tests/io/test_ascii_handler.py | 3 +- .../job_execution/test_job_manager.py | 5 +- .../layout/test_array_layout_utils.py | 111 +++++++++--------- .../test_derive_corsika_limits.py | 42 +++---- .../test_psf_parameter_optimisation.py | 25 ++-- .../simtel/test_simtel_io_metadata.py | 40 +++---- 7 files changed, 107 insertions(+), 122 deletions(-) diff --git a/tests/unit_tests/data_model/test_metadata_collector.py b/tests/unit_tests/data_model/test_metadata_collector.py index fa9f3b66e7..dab24483a6 100644 --- a/tests/unit_tests/data_model/test_metadata_collector.py +++ b/tests/unit_tests/data_model/test_metadata_collector.py @@ -14,7 +14,6 @@ import simtools.data_model.metadata_collector as metadata_collector from simtools.constants import METADATA_JSON_SCHEMA, SCHEMA_PATH from simtools.data_model import schema -from simtools.data_model.metadata_collector import MetadataCollector from simtools.utils import names logger = logging.getLogger() @@ -661,6 +660,6 @@ def mock_getuser(): def test_read_input_metadata_from_yml_or_json_no_file(): - collector = MetadataCollector(args_dict={}) + collector = metadata_collector.MetadataCollector(args_dict={}) with pytest.raises(FileNotFoundError, match=r"Failed reading metadata from missing_file\.yml"): collector._read_input_metadata_from_yml_or_json("missing_file.yml") diff --git a/tests/unit_tests/io/test_ascii_handler.py b/tests/unit_tests/io/test_ascii_handler.py index 5869bdf5dd..05122e3c22 100644 --- a/tests/unit_tests/io/test_ascii_handler.py +++ b/tests/unit_tests/io/test_ascii_handler.py @@ -12,7 +12,6 @@ import simtools.io.ascii_handler as ascii_handler from simtools.constants import MODEL_PARAMETER_METASCHEMA, MODEL_PARAMETER_SCHEMA_PATH -from simtools.io.ascii_handler import read_file_encoded_in_utf_or_latin FAILED_TO_READ_FILE_ERROR = r"^Failed to read file" url_simtools = "https://raw.githubusercontent.com/gammasim/simtools/main/" @@ -194,7 +193,7 @@ def test_read_file_encoded_in_utf_or_latin_unicode_decode_error(): with pytest.raises( UnicodeDecodeError, match=r"Unable to decode file.*using UTF-8 or Latin-1" ): - read_file_encoded_in_utf_or_latin(mock_file_name) + ascii_handler.read_file_encoded_in_utf_or_latin(mock_file_name) def test_json_numpy_encoder(): diff --git a/tests/unit_tests/job_execution/test_job_manager.py b/tests/unit_tests/job_execution/test_job_manager.py index 9db7441257..4c2fcfdfea 100644 --- a/tests/unit_tests/job_execution/test_job_manager.py +++ b/tests/unit_tests/job_execution/test_job_manager.py @@ -6,7 +6,6 @@ import pytest import simtools.job_execution.job_manager as jm -from simtools.job_execution.job_manager import JobExecutionError LOG_EXCERPT = "log excerpt" OS_SYSTEM = "os.system" @@ -117,7 +116,7 @@ def test_submit_local_real_failure( mock_subprocess = mocker.patch(subprocess_run) mock_subprocess.side_effect = subprocess.CalledProcessError(1, str(script_file)) - with pytest.raises(JobExecutionError, match="See excerpt from log file above"): + with pytest.raises(jm.JobExecutionError, match="See excerpt from log file above"): job_submitter_real.submit(script_file, output_log, logfile_log) job_submitter_real._logger.info.assert_any_call(job_messages["script_message"]) @@ -164,7 +163,7 @@ def test_submit_local_success( mock_subprocess_run = mocker.patch(subprocess_run) mock_subprocess_run.return_value.returncode = 42 with patch(builtins_open, mock_open(read_data="")): - with pytest.raises(JobExecutionError, match="Job submission failed with return code 42"): + with pytest.raises(jm.JobExecutionError, match="Job submission failed with return code 42"): job_submitter_real.submit(script_file, output_log, logfile_log) diff --git a/tests/unit_tests/layout/test_array_layout_utils.py b/tests/unit_tests/layout/test_array_layout_utils.py index f0b1394130..7e6f964121 100644 --- a/tests/unit_tests/layout/test_array_layout_utils.py +++ b/tests/unit_tests/layout/test_array_layout_utils.py @@ -6,14 +6,7 @@ import astropy.units as u import pytest -import simtools.layout.array_layout_utils as cta_array_layouts -from simtools.layout.array_layout_utils import ( - get_array_elements_from_db_for_layouts, - get_array_layouts_from_file, - get_array_layouts_from_parameter_file, - merge_array_layouts, - write_array_layouts, -) +from simtools.layout import array_layout_utils # Constants for patch paths PATCH_ASCII_COLLECT_FILE = "simtools.layout.array_layout_utils.ascii_handler.collect_data_from_file" @@ -71,7 +64,7 @@ def test_write_array_layouts( "output_path": test_path, "updated_parameter_version": "v1", } - write_array_layouts(array_layouts, args_dict) + array_layout_utils.write_array_layouts(array_layouts, args_dict) mock_io_handler.return_value.set_paths.assert_called_once_with(output_path=test_path) mock_io_handler.return_value.get_output_file.assert_called_once_with("array-layouts-v1.json") @@ -105,7 +98,7 @@ def test_merge_array_layouts(): ] # Call function - merged = merge_array_layouts(layouts_1, layouts_2) + merged = array_layout_utils.merge_array_layouts(layouts_1, layouts_2) # Assert results assert len(merged["value"]) == 3 @@ -136,19 +129,19 @@ def test_get_ctao_array_element_name(): "array_elements": [{"id": "T1", "name": "telescope1"}, {"id": "T2", "name": "telescope2"}] } - assert cta_array_layouts._get_ctao_array_element_name("T1", array_element_ids) == "telescope1" - assert cta_array_layouts._get_ctao_array_element_name("T2", array_element_ids) == "telescope2" + assert array_layout_utils._get_ctao_array_element_name("T1", array_element_ids) == "telescope1" + assert array_layout_utils._get_ctao_array_element_name("T2", array_element_ids) == "telescope2" # Test non-existent id - assert cta_array_layouts._get_ctao_array_element_name("T3", array_element_ids) is None + assert array_layout_utils._get_ctao_array_element_name("T3", array_element_ids) is None # Test empty array elements empty_elements = {"array_elements": []} - assert cta_array_layouts._get_ctao_array_element_name("T1", empty_elements) is None + assert array_layout_utils._get_ctao_array_element_name("T1", empty_elements) is None # Test missing array_elements key empty_dict = {} - assert cta_array_layouts._get_ctao_array_element_name("T1", empty_dict) is None + assert array_layout_utils._get_ctao_array_element_name("T1", empty_dict) is None @patch("simtools.layout.array_layout_utils.names") @@ -177,7 +170,7 @@ def test_get_ctao_layouts_per_site(mock_names): } # Call function - layouts = cta_array_layouts._get_ctao_layouts_per_site(site, sub_arrays, array_element_ids) + layouts = array_layout_utils._get_ctao_layouts_per_site(site, sub_arrays, array_element_ids) # Assert results assert len(layouts) == 1 @@ -185,18 +178,18 @@ def test_get_ctao_layouts_per_site(mock_names): assert layouts[0]["elements"] == ["N_tel1", "N_tel2"] # Test empty subarrays - layouts = cta_array_layouts._get_ctao_layouts_per_site( + layouts = array_layout_utils._get_ctao_layouts_per_site( site, {"subarrays": []}, array_element_ids ) assert len(layouts) == 0 # Test missing keys - layouts = cta_array_layouts._get_ctao_layouts_per_site(site, {}, array_element_ids) + layouts = array_layout_utils._get_ctao_layouts_per_site(site, {}, array_element_ids) assert len(layouts) == 0 # Test array with no matching elements site = "south" - layouts = cta_array_layouts._get_ctao_layouts_per_site( + layouts = array_layout_utils._get_ctao_layouts_per_site( site, {"subarrays": [{"name": "array1", "array_element_ids": ["T1", "T2"]}]}, array_element_ids, @@ -215,7 +208,7 @@ def test_retrieve_ctao_array_layouts_from_url(): mock_gen.is_url.return_value = True mock_ascii_handler.return_value = {"subarrays": [], "array_elements": []} - cta_array_layouts.retrieve_ctao_array_layouts( + array_layout_utils.retrieve_ctao_array_layouts( site="north", repository_url="https://test.com", branch_name="test-branch" ) @@ -232,7 +225,7 @@ def test_retrieve_ctao_array_layouts_from_file(test_path): mock_gen.is_url.return_value = False mock_ascii_handler.return_value = {"subarrays": [], "array_elements": []} - cta_array_layouts.retrieve_ctao_array_layouts( + array_layout_utils.retrieve_ctao_array_layouts( site="north", repository_url=test_path, branch_name="test-branch" ) @@ -251,7 +244,7 @@ def test_validate_array_layouts_with_db_valid(): ] } - result = cta_array_layouts.validate_array_layouts_with_db(production_table, array_layouts) + result = array_layout_utils.validate_array_layouts_with_db(production_table, array_layouts) assert result == array_layouts @@ -267,7 +260,7 @@ def test_validate_array_layouts_with_db_invalid(): } with pytest.raises(ValueError, match=r"Invalid array elements found: \['tel3', 'tel4'\]"): - cta_array_layouts.validate_array_layouts_with_db(production_table, array_layouts) + array_layout_utils.validate_array_layouts_with_db(production_table, array_layouts) def test_validate_array_layouts_with_db_empty_production_table(): @@ -281,7 +274,7 @@ def test_validate_array_layouts_with_db_empty_production_table(): } with pytest.raises(ValueError, match=r"Invalid array elements found: \['tel1'\]"): - cta_array_layouts.validate_array_layouts_with_db(production_table, array_layouts) + array_layout_utils.validate_array_layouts_with_db(production_table, array_layouts) def test_validate_array_layouts_with_db_empty_array_layouts(): @@ -290,7 +283,7 @@ def test_validate_array_layouts_with_db_empty_array_layouts(): array_layouts = {"value": []} - result = cta_array_layouts.validate_array_layouts_with_db(production_table, array_layouts) + result = array_layout_utils.validate_array_layouts_with_db(production_table, array_layouts) assert result == array_layouts @@ -305,13 +298,13 @@ def test_validate_array_layouts_with_db_missing_keys(): } with pytest.raises(ValueError, match=r"Invalid array elements found: \['tel1'\]"): - cta_array_layouts.validate_array_layouts_with_db(production_table, array_layouts) + array_layout_utils.validate_array_layouts_with_db(production_table, array_layouts) # Missing value key production_table = {"parameters": {"tel1": {}}} array_layouts = {} - result = cta_array_layouts.validate_array_layouts_with_db(production_table, array_layouts) + result = array_layout_utils.validate_array_layouts_with_db(production_table, array_layouts) assert result == array_layouts # Missing elements key in layout @@ -321,7 +314,7 @@ def test_validate_array_layouts_with_db_missing_keys(): ] } - result = cta_array_layouts.validate_array_layouts_with_db(production_table, array_layouts) + result = array_layout_utils.validate_array_layouts_with_db(production_table, array_layouts) assert result == array_layouts @@ -337,7 +330,7 @@ def test_validate_array_layouts_with_db_partial_invalid(): } with pytest.raises(ValueError, match=r"Invalid array elements found: \['tel3'\]"): - cta_array_layouts.validate_array_layouts_with_db(production_table, array_layouts) + array_layout_utils.validate_array_layouts_with_db(production_table, array_layouts) def test_get_array_layouts_from_parameter_file_valid(mocker, mock_array_model): @@ -357,7 +350,9 @@ def test_get_array_layouts_from_parameter_file_valid(mocker, mock_array_model): instance = mock_array_model.return_value instance.export_array_elements_as_table.return_value = fake_table - results = get_array_layouts_from_parameter_file("test_file.json", model_version) + results = array_layout_utils.get_array_layouts_from_parameter_file( + "test_file.json", model_version + ) assert isinstance(results, list) assert len(results) == 2 @@ -394,7 +389,7 @@ def test_get_array_layouts_from_parameter_file_missing_value_key(mocker): ) with pytest.raises(ValueError, match=r"Missing 'value' key in layout file."): - get_array_layouts_from_parameter_file("test_file.json", "6.0.0") + array_layout_utils.get_array_layouts_from_parameter_file("test_file.json", "6.0.0") def test_get_array_layouts_from_db_with_layout_name(mock_array_model): @@ -410,7 +405,7 @@ def test_get_array_layouts_from_db_with_layout_name(mock_array_model): mock_array_model.return_value = instance # Call the function with layout_name provided. - result = cta_array_layouts.get_array_layouts_from_db(layout_name, site, model_version) + result = array_layout_utils.get_array_layouts_from_db(layout_name, site, model_version) # Expected: a list with one dict corresponding to the provided layout_name. expected = { @@ -460,7 +455,7 @@ def array_model_side_effect(*args, **kwargs): mock_array_model.side_effect = array_model_side_effect # Call the function with layout_name as None. - result = cta_array_layouts.get_array_layouts_from_db(layout_name, site, model_version) + result = array_layout_utils.get_array_layouts_from_db(layout_name, site, model_version) # Expected: a list with two dicts. expected = [ @@ -503,7 +498,7 @@ def test_get_array_layouts_using_telescope_lists_from_db_with_site(mocker, mock_ instance.export_array_elements_as_table.return_value = fake_table mock_array_model.return_value = instance - results = cta_array_layouts.get_array_layouts_using_telescope_lists_from_db( + results = array_layout_utils.get_array_layouts_using_telescope_lists_from_db( telescope_lists, site, "6.0.0", coordinate_system="ground" ) @@ -533,7 +528,7 @@ def test_get_array_layouts_using_telescope_lists_from_db_without_site_single( instance.export_array_elements_as_table.return_value = fake_table mock_array_model.return_value = instance - results = cta_array_layouts.get_array_layouts_using_telescope_lists_from_db( + results = array_layout_utils.get_array_layouts_using_telescope_lists_from_db( telescope_lists, site, "6.1.0", coordinate_system="ground" ) @@ -556,7 +551,7 @@ def fake_get_site(name): mock_names.get_site_from_array_element_name.side_effect = fake_get_site with pytest.raises(ValueError, match="Telescope list contains elements from multiple sites:"): - cta_array_layouts.get_array_layouts_using_telescope_lists_from_db( + array_layout_utils.get_array_layouts_using_telescope_lists_from_db( telescope_lists, None, "6.2.0", coordinate_system="ground" ) @@ -565,7 +560,7 @@ def test_get_array_layouts_from_file_single_string(mocker, mock_read_table_from_ fake_table = ["dummy_table"] mocker.patch(mock_read_table_from_file, return_value=fake_table) file_path = "dummy_file.txt" - layouts = get_array_layouts_from_file(file_path) + layouts = array_layout_utils.get_array_layouts_from_file(file_path) assert len(layouts) == 1 expected_name = "dummy_file" # from "dummy_file.txt" assert layouts[0]["name"] == expected_name @@ -576,7 +571,7 @@ def test_get_array_layouts_from_file_single_path(mocker, mock_read_table_from_fi fake_table = ["path_table"] mocker.patch(mock_read_table_from_file, return_value=fake_table) file_path = Path("example_file.dat") - layouts = get_array_layouts_from_file(file_path) + layouts = array_layout_utils.get_array_layouts_from_file(file_path) assert len(layouts) == 1 expected_name = "example_file" # from "example_file.dat" assert layouts[0]["name"] == expected_name @@ -588,7 +583,7 @@ def test_get_array_layouts_from_file_list(mocker, mock_read_table_from_file): fake_table2 = ["table2"] mocker.patch(mock_read_table_from_file, side_effect=[fake_table1, fake_table2]) file_paths = ["file1.csv", "file2.csv"] - layouts = get_array_layouts_from_file(file_paths) + layouts = array_layout_utils.get_array_layouts_from_file(file_paths) assert len(layouts) == 2 assert layouts[0]["name"] == "file1" # from "file1.csv" assert layouts[1]["name"] == "file2" # from "file2.csv" @@ -610,7 +605,7 @@ def test_get_array_layout_dict_with_layout_name(mock_array_model): mock_array_model.return_value = instance # Call function - result = cta_array_layouts._get_array_layout_dict( + result = array_layout_utils._get_array_layout_dict( model_version, site, None, layout_name, "ground" ) @@ -643,7 +638,7 @@ def test_get_array_layout_dict_with_telescope_list(mock_array_model): mock_array_model.return_value = instance # Call function - result = cta_array_layouts._get_array_layout_dict( + result = array_layout_utils._get_array_layout_dict( model_version, site, telescope_list, None, "ground" ) @@ -674,7 +669,7 @@ def test_read_array_layouts_from_db_specific_layouts(mocker): site = "North" model_version = "v1.0.0" - result = get_array_elements_from_db_for_layouts(layouts, site, model_version) + result = array_layout_utils.get_array_elements_from_db_for_layouts(layouts, site, model_version) assert result == {"LST": [1, 2], "MST": [3, 4]} mock_site_model.assert_called_once_with(site=site, model_version=model_version) @@ -696,7 +691,7 @@ def test_read_array_layouts_from_db_all_layouts(mocker): site = "South" model_version = "v2.0.0" - result = get_array_elements_from_db_for_layouts(layouts, site, model_version) + result = array_layout_utils.get_array_elements_from_db_for_layouts(layouts, site, model_version) assert result == {"LST": [10, 20], "MST": [30, 40]} instance.get_list_of_array_layouts.assert_called_once() @@ -721,7 +716,7 @@ def minimal_args_dict(): def test_read_layouts_returns_empty_lists_when_no_inputs(minimal_args_dict): - layouts, background = cta_array_layouts.read_layouts(minimal_args_dict) + layouts, background = array_layout_utils.read_layouts(minimal_args_dict) assert layouts == [] assert background is None @@ -735,7 +730,7 @@ def test_read_layouts_with_array_layout_name_background(minimal_args_dict): {"array_elements": ["tel1", "tel2"]}, {"name": "main_layout", "site": "North", "array_elements": ["tel3", "tel4"]}, ] - layouts, background = cta_array_layouts.read_layouts(args) + layouts, background = array_layout_utils.read_layouts(args) assert background == ["tel1", "tel2"] assert isinstance(layouts, list) assert layouts[0]["name"] == "main_layout" @@ -769,7 +764,7 @@ def test_read_layouts_with_plot_all_layouts(minimal_args_dict): args["plot_all_layouts"] = True with patch("simtools.layout.array_layout_utils.get_array_layouts_from_db") as mock_get: mock_get.return_value = [{"name": "layout1", "array_elements": ["tel1"]}] - layouts, background = cta_array_layouts.read_layouts(args) + layouts, background = array_layout_utils.read_layouts(args) assert isinstance(layouts, list) assert layouts[0]["name"] == "layout1" assert background is None @@ -782,7 +777,7 @@ def test_read_layouts_with_array_layout_parameter_file(minimal_args_dict): "simtools.layout.array_layout_utils.get_array_layouts_from_parameter_file" ) as mock_get: mock_get.return_value = [{"name": "layout_param", "array_elements": ["telA"]}] - layouts, background = cta_array_layouts.read_layouts(args) + layouts, background = array_layout_utils.read_layouts(args) assert isinstance(layouts, list) assert layouts[0]["name"] == "layout_param" assert background is None @@ -793,7 +788,7 @@ def test_read_layouts_with_array_layout_file(minimal_args_dict): args["array_layout_file"] = "layout_file.txt" with patch("simtools.layout.array_layout_utils.get_array_layouts_from_file") as mock_get: mock_get.return_value = [{"name": "layout_file", "array_elements": ["telB"]}] - layouts, background = cta_array_layouts.read_layouts(args) + layouts, background = array_layout_utils.read_layouts(args) assert isinstance(layouts, list) assert layouts[0]["name"] == "layout_file" assert background is None @@ -806,7 +801,7 @@ def test_read_layouts_with_array_element_list(minimal_args_dict): "simtools.layout.array_layout_utils.get_array_layouts_using_telescope_lists_from_db" ) as mock_get: mock_get.return_value = [{"name": "list", "array_elements": ["telC", "telD"]}] - layouts, background = cta_array_layouts.read_layouts(args) + layouts, background = array_layout_utils.read_layouts(args) assert isinstance(layouts, list) assert layouts[0]["name"] == "list" assert background is None @@ -814,7 +809,7 @@ def test_read_layouts_with_array_element_list(minimal_args_dict): def test_create_regular_array_simple(): telescope_distance = {"MST": 100 * u.m} - table = cta_array_layouts.create_regular_array("1MST", "North", telescope_distance) + table = array_layout_utils.create_regular_array("1MST", "North", telescope_distance) assert len(table) == 1 assert table.meta["array_name"] == "1MST" assert table.meta["site"] == "North" @@ -830,7 +825,7 @@ def test_create_regular_array_four_telescopes(mocker): "simtools.layout.array_layout_utils.names.generate_array_element_name_from_type_site_id", side_effect=lambda tel_type, site, idx: f"{tel_type}_{site}_{idx}", ) - table = cta_array_layouts.create_regular_array("4MST", "South", telescope_distance) + table = array_layout_utils.create_regular_array("4MST", "South", telescope_distance) assert len(table) == 4 assert table.meta["array_name"] == "4MST" assert table.meta["site"] == "South" @@ -844,19 +839,19 @@ def test_create_regular_array_four_telescopes(mocker): assert z.value == 0 with pytest.raises(ValueError, match="Unsupported number of telescopes: 5"): - cta_array_layouts.create_regular_array("5MST", "South", telescope_distance) + array_layout_utils.create_regular_array("5MST", "South", telescope_distance) def test_get_array_name_valid(): - assert cta_array_layouts._get_array_name("4MST") == ("MST", 4) - assert cta_array_layouts._get_array_name("1LST") == ("LST", 1) - assert cta_array_layouts._get_array_name("2SST") == ("SST", 2) + assert array_layout_utils._get_array_name("4MST") == ("MST", 4) + assert array_layout_utils._get_array_name("1LST") == ("LST", 1) + assert array_layout_utils._get_array_name("2SST") == ("SST", 2) def test_get_array_name_invalid(): with pytest.raises(ValueError, match="Invalid array_name: 'MST'"): - cta_array_layouts._get_array_name("MST") + array_layout_utils._get_array_name("MST") with pytest.raises(ValueError, match="Invalid array_name: 'A4MST'"): - cta_array_layouts._get_array_name("A4MST") + array_layout_utils._get_array_name("A4MST") with pytest.raises(ValueError, match="Invalid array_name: ''"): - cta_array_layouts._get_array_name("") + array_layout_utils._get_array_name("") diff --git a/tests/unit_tests/production_configuration/test_derive_corsika_limits.py b/tests/unit_tests/production_configuration/test_derive_corsika_limits.py index 6b04b39196..03bc60e046 100644 --- a/tests/unit_tests/production_configuration/test_derive_corsika_limits.py +++ b/tests/unit_tests/production_configuration/test_derive_corsika_limits.py @@ -4,12 +4,6 @@ from astropy.table import Table import simtools.production_configuration.derive_corsika_limits as derive_corsika_limits -from simtools.production_configuration.derive_corsika_limits import ( - _create_results_table, - _round_value, - generate_corsika_limits_grid, - write_results, -) # Constants SIM_EVENTS_HISTOGRAMS_PATH = ( @@ -78,7 +72,7 @@ def test_generate_corsika_limits_grid(mocker, mock_args_dict): ) # Run function - generate_corsika_limits_grid(mock_args_dict) + derive_corsika_limits.generate_corsika_limits_grid(mock_args_dict) # Verify calls assert mock_collect.call_count == 1 @@ -134,7 +128,7 @@ def test_write_results(mocker, mock_args_dict, mock_results, tmp_test_directory) mock_dump = mocker.patch("simtools.data_model.metadata_collector.MetadataCollector.dump") - write_results(mock_results, mock_args_dict) + derive_corsika_limits.write_results(mock_results, mock_args_dict) # Verify metadata was written mock_dump.assert_called_once() @@ -144,7 +138,7 @@ def test_write_results(mocker, mock_args_dict, mock_results, tmp_test_directory) def test_create_results_table(mock_results): """Test _create_results_table function.""" - table = _create_results_table(mock_results, loss_fraction=0.2) + table = derive_corsika_limits._create_results_table(mock_results, loss_fraction=0.2) table.info() assert isinstance(table, Table) @@ -160,26 +154,26 @@ def test_round_value(): """Test _round_value function for different key types.""" # Test lower_energy_limit rounding - assert _round_value("lower_energy_limit", 1.2345) == pytest.approx(1.234) - assert _round_value("lower_energy_limit", 0.9876) == pytest.approx(0.987) - assert _round_value("lower_energy_limit", 2.0) == pytest.approx(2.0) + assert derive_corsika_limits._round_value("lower_energy_limit", 1.2345) == pytest.approx(1.234) + assert derive_corsika_limits._round_value("lower_energy_limit", 0.9876) == pytest.approx(0.987) + assert derive_corsika_limits._round_value("lower_energy_limit", 2.0) == pytest.approx(2.0) # Test upper_radius_limit rounding - assert _round_value("upper_radius_limit", 123.4) == 125 - assert _round_value("upper_radius_limit", 100.0) == 100 - assert _round_value("upper_radius_limit", 101.0) == 125 - assert _round_value("upper_radius_limit", 75.0) == 75 + assert derive_corsika_limits._round_value("upper_radius_limit", 123.4) == 125 + assert derive_corsika_limits._round_value("upper_radius_limit", 100.0) == 100 + assert derive_corsika_limits._round_value("upper_radius_limit", 101.0) == 125 + assert derive_corsika_limits._round_value("upper_radius_limit", 75.0) == 75 # Test viewcone_radius rounding - assert _round_value("viewcone_radius", 1.1) == pytest.approx(1.25) - assert _round_value("viewcone_radius", 2.0) == pytest.approx(2.0) - assert _round_value("viewcone_radius", 2.1) == pytest.approx(2.25) - assert _round_value("viewcone_radius", 0.3) == pytest.approx(0.5) + assert derive_corsika_limits._round_value("viewcone_radius", 1.1) == pytest.approx(1.25) + assert derive_corsika_limits._round_value("viewcone_radius", 2.0) == pytest.approx(2.0) + assert derive_corsika_limits._round_value("viewcone_radius", 2.1) == pytest.approx(2.25) + assert derive_corsika_limits._round_value("viewcone_radius", 0.3) == pytest.approx(0.5) # Test other keys (no rounding) - assert _round_value("other_key", 1.2345) == pytest.approx(1.2345) - assert _round_value("zenith", 45.678) == pytest.approx(45.678) - assert _round_value("unknown", "string_value") == "string_value" + assert derive_corsika_limits._round_value("other_key", 1.2345) == pytest.approx(1.2345) + assert derive_corsika_limits._round_value("zenith", 45.678) == pytest.approx(45.678) + assert derive_corsika_limits._round_value("unknown", "string_value") == "string_value" def test_generate_corsika_limits_grid_with_db_layouts(mocker, mock_args_dict): @@ -203,7 +197,7 @@ def test_generate_corsika_limits_grid_with_db_layouts(mocker, mock_args_dict): "simtools.production_configuration.derive_corsika_limits.write_results" ) - generate_corsika_limits_grid(args) + derive_corsika_limits.generate_corsika_limits_grid(args) mock_read_layouts.assert_called_once_with( args["array_layout_name"], args["site"], args["model_version"] diff --git a/tests/unit_tests/ray_tracing/test_psf_parameter_optimisation.py b/tests/unit_tests/ray_tracing/test_psf_parameter_optimisation.py index ec4f62eafa..dabf0f5473 100644 --- a/tests/unit_tests/ray_tracing/test_psf_parameter_optimisation.py +++ b/tests/unit_tests/ray_tracing/test_psf_parameter_optimisation.py @@ -8,7 +8,6 @@ import pytest import simtools.ray_tracing.psf_parameter_optimisation as psf_opt -from simtools.ray_tracing.psf_parameter_optimisation import GradientStepResult TEST_OUTPUT_DIR = Path("/dummy_test_path") @@ -561,7 +560,7 @@ def test_perform_gradient_step_with_retries(optimizer): ) assert result is not None - assert isinstance(result, GradientStepResult) + assert isinstance(result, psf_opt.GradientStepResult) assert result.params == {"mirror_reflection_random_angle": [0.004]} assert result.step_accepted is True @@ -982,7 +981,7 @@ def test_gradient_descent_convergence_and_tracking(optimizer, sample_data): mock_get_params.return_value = {"mirror_reflection_random_angle": [0.005, 0.15, 0.03]} mock_sim.return_value = (3.5, 0.05, 0.8, sample_data) mock_step.side_effect = [ - GradientStepResult( + psf_opt.GradientStepResult( params={"mirror_reflection_random_angle": [0.004, 0.15, 0.028]}, psf_diameter=3.4, metric=0.008, @@ -1006,7 +1005,7 @@ def test_gradient_descent_convergence_and_tracking(optimizer, sample_data): ): mock_get_params.return_value = {"mirror_reflection_random_angle": [0.005, 0.15, 0.03]} mock_sim.return_value = (3.5, 0.1, 0.8, sample_data) - mock_step.return_value = GradientStepResult( + mock_step.return_value = psf_opt.GradientStepResult( params={"mirror_reflection_random_angle": [0.004, 0.15, 0.028]}, psf_diameter=3.4, metric=0.095, @@ -1030,7 +1029,7 @@ def test_gradient_descent_convergence_and_tracking(optimizer, sample_data): mock_get_params.return_value = {"mirror_reflection_random_angle": [0.005, 0.15, 0.03]} mock_sim.return_value = (3.5, 0.1, 0.8, sample_data) mock_step.side_effect = [ - GradientStepResult( + psf_opt.GradientStepResult( params={"mirror_reflection_random_angle": [0.004, 0.15, 0.028]}, psf_diameter=3.4, metric=0.08, @@ -1039,7 +1038,7 @@ def test_gradient_descent_convergence_and_tracking(optimizer, sample_data): step_accepted=True, learning_rate=0.1, ), # Better - GradientStepResult( + psf_opt.GradientStepResult( params={"mirror_reflection_random_angle": [0.003, 0.15, 0.025]}, psf_diameter=3.3, metric=0.05, @@ -1048,7 +1047,7 @@ def test_gradient_descent_convergence_and_tracking(optimizer, sample_data): step_accepted=True, learning_rate=0.1, ), # Best - GradientStepResult( + psf_opt.GradientStepResult( params={"mirror_reflection_random_angle": [0.002, 0.15, 0.020]}, psf_diameter=3.2, metric=0.12, @@ -1092,7 +1091,7 @@ def test_perform_gradient_step_with_retries_learning_rate_reduction( ) # Should fail and return GradientStepResult with False for step_accepted - assert isinstance(result, GradientStepResult) + assert isinstance(result, psf_opt.GradientStepResult) assert result.step_accepted is False @@ -1451,7 +1450,7 @@ def test_run_gradient_descent_no_step_accepted(optimizer, sample_data): # First call: step not accepted # Second call: step accepted mock_step.side_effect = [ - GradientStepResult( + psf_opt.GradientStepResult( params=None, psf_diameter=None, metric=None, @@ -1460,7 +1459,7 @@ def test_run_gradient_descent_no_step_accepted(optimizer, sample_data): step_accepted=False, learning_rate=0.1, ), - GradientStepResult( + psf_opt.GradientStepResult( params={"mirror_reflection_random_angle": [0.004, 0.15, 0.028]}, psf_diameter=3.4, metric=0.008, @@ -1491,7 +1490,7 @@ def test_run_gradient_descent_learning_rate_cap(optimizer, sample_data): # Multiple steps not accepted to trigger learning rate increases mock_step.side_effect = [ - GradientStepResult( + psf_opt.GradientStepResult( params=None, psf_diameter=None, metric=None, @@ -1500,7 +1499,7 @@ def test_run_gradient_descent_learning_rate_cap(optimizer, sample_data): step_accepted=False, learning_rate=0.5, ), - GradientStepResult( + psf_opt.GradientStepResult( params=None, psf_diameter=None, metric=None, @@ -1509,7 +1508,7 @@ def test_run_gradient_descent_learning_rate_cap(optimizer, sample_data): step_accepted=False, learning_rate=1.0, ), - GradientStepResult( + psf_opt.GradientStepResult( params={"mirror_reflection_random_angle": [0.004, 0.15, 0.028]}, psf_diameter=3.4, metric=0.008, diff --git a/tests/unit_tests/simtel/test_simtel_io_metadata.py b/tests/unit_tests/simtel/test_simtel_io_metadata.py index 30ace1cd12..f2c28712a3 100644 --- a/tests/unit_tests/simtel/test_simtel_io_metadata.py +++ b/tests/unit_tests/simtel/test_simtel_io_metadata.py @@ -5,20 +5,13 @@ import pytest import simtools.simtel.simtel_io_metadata as simtel_io_metadata -from simtools.simtel.simtel_io_metadata import ( - _decode_dictionary, - _guess_telescope_name_for_legacy_files, - get_sim_telarray_telescope_id, - get_sim_telarray_telescope_id_to_telescope_name_mapping, - read_sim_telarray_metadata, -) def test_decode_success(): test_meta = {b"key1": b"value1", b"key2": b"value2"} - result = _decode_dictionary(test_meta) + result = simtel_io_metadata._decode_dictionary(test_meta) assert result == {"key1": "value1", "key2": "value2"} - result = _decode_dictionary(test_meta, encoding="ascii") + result = simtel_io_metadata._decode_dictionary(test_meta, encoding="ascii") assert result == {"key1": "value1", "key2": "value2"} @@ -26,7 +19,7 @@ def test_decode_with_unicode_error(caplog): # Create metadata with invalid unicode bytes test_meta = {b"key1": b"value1", b"key2": b"\xff\xfe invalid utf8"} - result = _decode_dictionary(test_meta, encoding="utf-8") + result = simtel_io_metadata._decode_dictionary(test_meta, encoding="utf-8") assert "key1" in result assert "key2" in result @@ -36,7 +29,9 @@ def test_decode_with_unicode_error(caplog): def test_read_sim_telarray_metadata(sim_telarray_file_gamma): - global_meta, telescope_meta = read_sim_telarray_metadata(sim_telarray_file_gamma) + global_meta, telescope_meta = simtel_io_metadata.read_sim_telarray_metadata( + sim_telarray_file_gamma + ) assert global_meta is not None assert len(telescope_meta) > 0 assert isinstance(telescope_meta, dict) @@ -56,18 +51,21 @@ def test_read_sim_telarray_metadata(sim_telarray_file_gamma): def test_read_sim_telarray_metadata_attribute_error(mock_decode, sim_telarray_file_gamma): simtel_io_metadata.read_sim_telarray_metadata.cache_clear() with pytest.raises(AttributeError, match=r"^Error reading metadata from file"): - read_sim_telarray_metadata(sim_telarray_file_gamma) + simtel_io_metadata.read_sim_telarray_metadata(sim_telarray_file_gamma) def test_get_sim_telarray_telescope_id(sim_telarray_file_gamma): - assert get_sim_telarray_telescope_id("LSTN-01", sim_telarray_file_gamma) == 1 - assert get_sim_telarray_telescope_id("MSTN-01", sim_telarray_file_gamma) == 5 - assert get_sim_telarray_telescope_id("MSTS-01", sim_telarray_file_gamma) is None + assert simtel_io_metadata.get_sim_telarray_telescope_id("LSTN-01", sim_telarray_file_gamma) == 1 + assert simtel_io_metadata.get_sim_telarray_telescope_id("MSTN-01", sim_telarray_file_gamma) == 5 + assert ( + simtel_io_metadata.get_sim_telarray_telescope_id("MSTS-01", sim_telarray_file_gamma) is None + ) def test_get_sim_telarray_telescope_id_to_telescope_name_mapping(sim_telarray_file_gamma): - tel_mapping = get_sim_telarray_telescope_id_to_telescope_name_mapping(sim_telarray_file_gamma) - + tel_mapping = simtel_io_metadata.get_sim_telarray_telescope_id_to_telescope_name_mapping( + sim_telarray_file_gamma + ) assert isinstance(tel_mapping, dict) assert len(tel_mapping) > 0 assert all(isinstance(k, int) for k in tel_mapping.keys()) @@ -171,11 +169,11 @@ def test_guess_telescope_name_for_legacy_files(monkeypatch): ) # Should return the correct validated name for index 1 - result = _guess_telescope_name_for_legacy_files(1, "dummy5.simtel") + result = simtel_io_metadata._guess_telescope_name_for_legacy_files(1, "dummy5.simtel") assert result == "MSTN-02" # Should return None for out-of-range index - result_none = _guess_telescope_name_for_legacy_files(10, "dummy5.simtel") + result_none = simtel_io_metadata._guess_telescope_name_for_legacy_files(10, "dummy5.simtel") assert result_none is None @@ -195,5 +193,7 @@ def test_get_sim_telarray_telescope_id_to_telescope_name_mapping_value_error(mon "simtools.simtel.simtel_io_metadata.read_sim_telarray_metadata", lambda file: ({}, {1: {"optics_config_name": "bad"}, 2: {"optics_config_name": "bad2"}}), ) - mapping = get_sim_telarray_telescope_id_to_telescope_name_mapping("dummy4.simtel") + mapping = simtel_io_metadata.get_sim_telarray_telescope_id_to_telescope_name_mapping( + "dummy4.simtel" + ) assert mapping == {1: "FAKE-0", 2: "FAKE-1"} From b06882c26e756f31f23806aed9efe0417a034927 Mon Sep 17 00:00:00 2001 From: Gernot Maier Date: Fri, 19 Dec 2025 17:25:40 +0100 Subject: [PATCH 7/7] changelog --- docs/changes/1959.maintenance.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/changes/1959.maintenance.md diff --git a/docs/changes/1959.maintenance.md b/docs/changes/1959.maintenance.md new file mode 100644 index 0000000000..0af1359a19 --- /dev/null +++ b/docs/changes/1959.maintenance.md @@ -0,0 +1 @@ +Minor code quality improvements reported by GitHub Code QL.