From dbc49dc709ba6ce21f9600c9bd4eacca98df9d0c Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 13:34:36 -0800 Subject: [PATCH 01/11] remove changes from user brubbel --- kwave/executor.py | 5 +- kwave/kspaceFirstOrder2D.py | 2 +- kwave/kspaceFirstOrder3D.py | 2 +- kwave/kspaceFirstOrderAS.py | 2 +- kwave/options/simulation_execution_options.py | 51 +++++++++---------- 5 files changed, 30 insertions(+), 32 deletions(-) diff --git a/kwave/executor.py b/kwave/executor.py index 949c0c37..de8cd4cb 100644 --- a/kwave/executor.py +++ b/kwave/executor.py @@ -32,9 +32,8 @@ def _make_binary_executable(self): raise FileNotFoundError(f"Binary not found at {binary_path}") binary_path.chmod(binary_path.stat().st_mode | stat.S_IEXEC) - def run_simulation(self, input_filename: str, output_filename: str, options: str): - command = [str(self.execution_options.binary_path), "-i", input_filename, "-o", output_filename] - command.extend(options.split(' ')) + def run_simulation(self, input_filename: str, output_filename: str, options: list[str]) -> dotdict: + command = [str(self.execution_options.binary_path), "-i", input_filename, "-o", output_filename] + options try: with subprocess.Popen( diff --git a/kwave/kspaceFirstOrder2D.py b/kwave/kspaceFirstOrder2D.py index 536a8e97..6efa9d93 100644 --- a/kwave/kspaceFirstOrder2D.py +++ b/kwave/kspaceFirstOrder2D.py @@ -440,6 +440,6 @@ def kspaceFirstOrder2D( return executor = Executor(simulation_options=simulation_options, execution_options=execution_options) - executor_options = execution_options.get_options_string(sensor=k_sim.sensor) + executor_options = execution_options.as_list(sensor=k_sim.sensor) sensor_data = executor.run_simulation(k_sim.options.input_filename, k_sim.options.output_filename, options=executor_options) return sensor_data diff --git a/kwave/kspaceFirstOrder3D.py b/kwave/kspaceFirstOrder3D.py index 345426e6..5567fccf 100644 --- a/kwave/kspaceFirstOrder3D.py +++ b/kwave/kspaceFirstOrder3D.py @@ -461,6 +461,6 @@ def kspaceFirstOrder3D( return executor = Executor(simulation_options=simulation_options, execution_options=execution_options) - executor_options = execution_options.get_options_string(sensor=k_sim.sensor) + executor_options = execution_options.as_list(sensor=k_sim.sensor) sensor_data = executor.run_simulation(k_sim.options.input_filename, k_sim.options.output_filename, options=executor_options) return sensor_data diff --git a/kwave/kspaceFirstOrderAS.py b/kwave/kspaceFirstOrderAS.py index 70c7d2ff..0c8c2cd1 100644 --- a/kwave/kspaceFirstOrderAS.py +++ b/kwave/kspaceFirstOrderAS.py @@ -365,6 +365,6 @@ def kspaceFirstOrderAS( return executor = Executor(simulation_options=simulation_options, execution_options=execution_options) - executor_options = execution_options.get_options_string(sensor=k_sim.sensor) + executor_options = execution_options.as_list(sensor=k_sim.sensor) sensor_data = executor.run_simulation(k_sim.options.input_filename, k_sim.options.output_filename, options=executor_options) return sensor_data diff --git a/kwave/options/simulation_execution_options.py b/kwave/options/simulation_execution_options.py index 07b4ab5b..d4b4b0cb 100644 --- a/kwave/options/simulation_execution_options.py +++ b/kwave/options/simulation_execution_options.py @@ -1,6 +1,7 @@ from pathlib import Path from typing import Optional, Union import os +import warnings from kwave import PLATFORM, BINARY_DIR from kwave.ksensor import kSensor @@ -149,51 +150,49 @@ def binary_dir(self, value: str): ) self._binary_dir = Path(value) - def get_options_string(self, sensor: kSensor) -> str: + def as_list(self, sensor: kSensor) -> list[str]: options_list = [] - if self.device_num is not None and self.device_num < 0: - raise ValueError("Device number must be non-negative") + if self.device_num is not None: - options_list.append(f" -g {self.device_num}") + if self.device_num < 0: + raise ValueError("Device number must be non-negative") + options_list.append(f"-g {self.device_num}") if self.num_threads is not None and PLATFORM != "windows": - options_list.append(f" -t {self.num_threads}") + options_list.append(f"-t {self.num_threads}") if self.verbose_level > 0: - options_list.append(f" --verbose {self.verbose_level}") + options_list.append(f"--verbose {self.verbose_level}") record_options_map = { - "p": "p_raw", - "p_max": "p_max", - "p_min": "p_min", - "p_rms": "p_rms", - "p_max_all": "p_max_all", - "p_min_all": "p_min_all", - "p_final": "p_final", - "u": "u_raw", - "u_max": "u_max", - "u_min": "u_min", - "u_rms": "u_rms", - "u_max_all": "u_max_all", - "u_min_all": "u_min_all", - "u_final": "u_final", + "p": "p_raw", "p_max": "p_max", "p_min": "p_min", "p_rms": "p_rms", + "p_max_all": "p_max_all", "p_min_all": "p_min_all", "p_final": "p_final", + "u": "u_raw", "u_max": "u_max", "u_min": "u_min", "u_rms": "u_rms", + "u_max_all": "u_max_all", "u_min_all": "u_min_all", "u_final": "u_final", } if sensor.record is not None: matching_keys = set(sensor.record).intersection(record_options_map.keys()) - for key in matching_keys: - options_list.append(f" --{record_options_map[key]}") + options_list.extend([f"--{record_options_map[key]}" for key in matching_keys]) if "u_non_staggered" in sensor.record or "I_avg" in sensor.record or "I" in sensor.record: - options_list.append(" --u_non_staggered_raw") + options_list.append("--u_non_staggered_raw") if ("I_avg" in sensor.record or "I" in sensor.record) and ("p" not in sensor.record): - options_list.append(" --p_raw") + options_list.append("--p_raw") else: - options_list.append(" --p_raw") + options_list.append("--p_raw") if sensor.record_start_index is not None: - options_list.append(f" -s {sensor.record_start_index}") + options_list.append(f"-s {sensor.record_start_index}") + + return options_list + + + def get_options_string(self, sensor: kSensor) -> str: + # raise a deprication warning + warnings.warn("This method is deprecated. Use `as_list` method instead.", DeprecationWarning) + options_list = self.as_list(sensor) return " ".join(options_list) From 168d812142066e1ff5938f62be1c0167edc72051 Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 13:53:09 -0800 Subject: [PATCH 02/11] update executor test --- tests/test_executor.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_executor.py b/tests/test_executor.py index b3414fea..c1826c4f 100644 --- a/tests/test_executor.py +++ b/tests/test_executor.py @@ -78,7 +78,7 @@ def mock_stdout_gen(): # Mock the parse_executable_output method with patch.object(executor, "parse_executable_output", return_value=dotdict()): - sensor_data = executor.run_simulation("input.h5", "output.h5", "options") + sensor_data = executor.run_simulation("input.h5", "output.h5", ["options"]) # Assert that the print function was called with the expected lines expected_calls = [call("line 1\n", end=""), call("line 2\n", end=""), call("line 3\n", end="")] @@ -96,7 +96,7 @@ def test_run_simulation_success(self): # Mock the parse_executable_output method with patch.object(executor, "parse_executable_output", return_value=dotdict()): - sensor_data = executor.run_simulation("input.h5", "output.h5", "options") + sensor_data = executor.run_simulation("input.h5", "output.h5", ["options"]) normalized_path = os.path.normpath(self.execution_options.binary_path) expected_command = [normalized_path, "-i", "input.h5", "-o", "output.h5", "options"] @@ -119,7 +119,7 @@ def test_run_simulation_failure(self): # Mock the parse_executable_output method with patch.object(executor, "parse_executable_output", return_value=dotdict()): with self.assertRaises(subprocess.CalledProcessError): - executor.run_simulation("input.h5", "output.h5", "options") + executor.run_simulation("input.h5", "output.h5", ["options"]) # Get the printed output stdout_output = mock_stdout.getvalue() From cdbb4cd6248c90029f1ec100e612dc0b04da44fe Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 14:09:18 -0800 Subject: [PATCH 03/11] ignore deprecation warnings in k-wave-python --- pyproject.toml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index b124830a..f028e406 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -80,8 +80,10 @@ exclude = [ testpaths = ["tests"] filterwarnings = [ "error::DeprecationWarning", - "error::PendingDeprecationWarning" + "error::PendingDeprecationWarning", + "ignore::DeprecationWarning:kwave", ] + [tool.coverage.run] branch = true command_line = "-m pytest" From 957517f4df79e90bafade95b7b4124fbbc14a313 Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 14:16:36 -0800 Subject: [PATCH 04/11] update comparison strings --- tests/test_simulation_execution_options.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 1646bcce..7cdbec8d 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -95,7 +95,7 @@ def test_get_options_string_darwin(self): options.verbose_level = 2 options_string = options.get_options_string(self.mock_sensor) - expected_substrings = [" -g 1", f" -t {os.cpu_count()}", " --verbose 2", " --p_raw", " --u_max", " -s 10"] + expected_substrings = ["-g 1", f"-t {os.cpu_count()}", "--verbose 2", "--p_raw", "--u_max", "-s 10"] for substring in expected_substrings: self.assertIn(substring, options_string) @@ -108,7 +108,7 @@ def test_get_options_string_windows(self): options.verbose_level = 2 options_string = options.get_options_string(self.mock_sensor) - expected_substrings = [" -g 1", " --verbose 2", " --p_raw", " --u_max", " -s 10"] + expected_substrings = ["-g 1", "--verbose 2", "--p_raw", "--u_max", "-s 10"] for substring in expected_substrings: self.assertIn(substring, options_string) self.assertNotIn(f" -t {os.cpu_count()}", expected_substrings) @@ -122,7 +122,7 @@ def test_get_options_string_linux(self): options.verbose_level = 2 options_string = options.get_options_string(self.mock_sensor) - expected_substrings = [" -g 1", f" -t {os.cpu_count()}", " --verbose 2", " --p_raw", " --u_max", " -s 10"] + expected_substrings = ["-g 1", f"-t {os.cpu_count()}", "--verbose 2", "--p_raw", "--u_max", "-s 10"] for substring in expected_substrings: self.assertIn(substring, options_string) From 275b4de48c3462e08ececa649fff3c96affea84e Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 14:31:02 -0800 Subject: [PATCH 05/11] improve coverage --- tests/test_simulation_execution_options.py | 145 +++++++++++++-------- 1 file changed, 91 insertions(+), 54 deletions(-) diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 7cdbec8d..7eb8905c 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -94,75 +94,112 @@ def test_get_options_string_darwin(self): options.num_threads = os.cpu_count() options.verbose_level = 2 - options_string = options.get_options_string(self.mock_sensor) - expected_substrings = ["-g 1", f"-t {os.cpu_count()}", "--verbose 2", "--p_raw", "--u_max", "-s 10"] - for substring in expected_substrings: - self.assertIn(substring, options_string) + options_list = options.as_list(self.mock_sensor) + expected_elements = [ + f"-g {options.device_num}", + f"-t {os.cpu_count()}", + "--verbose 2", + "--p_raw", + "--u_max", + f"-s {self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor + ] + for element in expected_elements: + self.assertIn(element, options_list) @patch("kwave.options.simulation_execution_options.PLATFORM", "windows") - def test_get_options_string_windows(self): - """Test the get_options_string method with a mock sensor.""" + def test_as_list_windows(self): + """Test the list representation of options on Windows.""" options = self.default_options options.device_num = 1 options.num_threads = os.cpu_count() options.verbose_level = 2 - options_string = options.get_options_string(self.mock_sensor) - expected_substrings = ["-g 1", "--verbose 2", "--p_raw", "--u_max", "-s 10"] - for substring in expected_substrings: - self.assertIn(substring, options_string) - self.assertNotIn(f" -t {os.cpu_count()}", expected_substrings) + options_list = options.as_list(self.mock_sensor) + expected_elements = [ + f"-g {options.device_num}", + "--verbose 2", + "--p_raw", + "--u_max", + f"-s {self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor + ] + for element in expected_elements: + self.assertIn(element, options_list) + self.assertNotIn(f"-t {os.cpu_count()}", options_list) - @patch("kwave.options.simulation_execution_options.PLATFORM", "linux") - def test_get_options_string_linux(self): - """Test the get_options_string method with a mock sensor.""" + @patch("kwave.options.simulation_execution_options.PLATFORM", "darwin") + def test_as_list_darwin(self): + """Test the list representation of options on macOS.""" options = self.default_options options.device_num = 1 options.num_threads = os.cpu_count() options.verbose_level = 2 - options_string = options.get_options_string(self.mock_sensor) - expected_substrings = ["-g 1", f"-t {os.cpu_count()}", "--verbose 2", "--p_raw", "--u_max", "-s 10"] - for substring in expected_substrings: - self.assertIn(substring, options_string) - - def test_gpu_dependency_on_binary_name_and_path(self): - """Test that the binary_name and binary_path are updated correctly based on is_gpu_simulation.""" - options = SimulationExecutionOptions(is_gpu_simulation=True) - self.assertEqual(options.binary_name, CUDA_BINARY_NAME) - - options.is_gpu_simulation = False - self.assertEqual(options.binary_name, OMP_BINARY_NAME) - self.assertTrue(str(options.binary_path).endswith(OMP_BINARY_NAME)) + options_list = options.as_list(self.mock_sensor) + expected_elements = [ + f"-g {options.device_num}", + f"-t {os.cpu_count()}", + "--verbose 2", + "--p_raw", + "--u_max", + f"-s {self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor + ] + for element in expected_elements: + self.assertIn(element, options_list) + + def test_as_list_custom_record(self): + """Test the list representation with a custom record configuration.""" + options = self.default_options + self.mock_sensor.record = ["p_max", "u_min", "I_avg"] + options.device_num = 2 + options.num_threads = 8 + options.verbose_level = 1 + + options_list = options.as_list(self.mock_sensor) + expected_elements = [ + f"-g {options.device_num}", + "-t 8", + "--verbose 1", + "--p_max", + "--u_min", + "--p_raw" # Default if no specific 'p' or 'u' options are given + ] + for element in expected_elements: + self.assertIn(element, options_list) + + def test_as_list_with_invalid_values(self): + """Test the behavior of as_list when there are invalid values.""" + options = self.default_options + options.device_num = -1 # Invalid device_num should raise an exception when set + with self.assertRaises(ValueError): + options.as_list(self.mock_sensor) - def test_env_vars_linux(self): - with patch("kwave.options.simulation_execution_options.PLATFORM", "linux"): - options = SimulationExecutionOptions() - env_vars = options.env_vars - self.assertIn("OMP_PLACES", env_vars) - self.assertEqual(env_vars["OMP_PLACES"], "cores") - self.assertIn("OMP_PROC_BIND", env_vars) - self.assertEqual(env_vars["OMP_PROC_BIND"], "SPREAD") - - def test_thread_binding_linux(self): - with patch("kwave.options.simulation_execution_options.PLATFORM", "linux"): - options = SimulationExecutionOptions(thread_binding=True) - env_vars = options.env_vars - self.assertEqual(env_vars["OMP_PROC_BIND"], "SPREAD") - - def test_thread_binding_darwin(self): - with patch("kwave.options.simulation_execution_options.PLATFORM", "darwin"): - options = SimulationExecutionOptions(thread_binding=True) - with self.assertRaises(ValueError, msg="Thread binding is not supported in MacOS."): - _ = options.env_vars - - def test_env_vars_darwin(self): - with patch("kwave.options.simulation_execution_options.PLATFORM", "darwin"): - options = SimulationExecutionOptions() - env_vars = options.env_vars - self.assertNotIn("OMP_PLACES", env_vars) - self.assertNotIn("OMP_PROC_BIND", env_vars) + def test_as_list_no_record(self): + """Test the list representation when there is no record.""" + options = self.default_options + self.mock_sensor.record = None + options.device_num = 1 + options.num_threads = 4 + options.verbose_level = 0 + + options_list = options.as_list(self.mock_sensor) + expected_elements = [ + f"-g {options.device_num}", + "-t 4", + "--p_raw", # Default value + ] + for element in expected_elements: + self.assertIn(element, options_list) + + def test_list_compared_to_string(self): + """Test the list representation compared to the string representation.""" + options = self.default_options + options.device_num = 1 + options.num_threads = 4 + options.verbose_level = 1 + options_list = options.as_list(self.mock_sensor) + options_string = options.get_options_string(self.mock_sensor) + self.assertEqual(" ".join(options_list), options_string) if __name__ == "__main__": unittest.main() From 1faee26f656bc17053db7c68ba5cff62445b4956 Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 14:50:56 -0800 Subject: [PATCH 06/11] split arguments --- kwave/options/simulation_execution_options.py | 13 +++-- tests/test_simulation_execution_options.py | 48 ++++++++++++------- 2 files changed, 41 insertions(+), 20 deletions(-) diff --git a/kwave/options/simulation_execution_options.py b/kwave/options/simulation_execution_options.py index d4b4b0cb..07d1e9b2 100644 --- a/kwave/options/simulation_execution_options.py +++ b/kwave/options/simulation_execution_options.py @@ -156,13 +156,17 @@ def as_list(self, sensor: kSensor) -> list[str]: if self.device_num is not None: if self.device_num < 0: raise ValueError("Device number must be non-negative") - options_list.append(f"-g {self.device_num}") + options_list.append("-g") + options_list.append(str(self.device_num)) if self.num_threads is not None and PLATFORM != "windows": - options_list.append(f"-t {self.num_threads}") + options_list.append("-t") + options_list.append(str(self.num_threads)) if self.verbose_level > 0: - options_list.append(f"--verbose {self.verbose_level}") + options_list.append("--verbose") + options_list.append(str(self.verbose_level)) + record_options_map = { "p": "p_raw", "p_max": "p_max", "p_min": "p_min", "p_rms": "p_rms", @@ -184,7 +188,8 @@ def as_list(self, sensor: kSensor) -> list[str]: options_list.append("--p_raw") if sensor.record_start_index is not None: - options_list.append(f"-s {sensor.record_start_index}") + options_list.append("-s") + options_list.append(f"{sensor.record_start_index}") return options_list diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 7eb8905c..8a25de9b 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -96,12 +96,16 @@ def test_get_options_string_darwin(self): options_list = options.as_list(self.mock_sensor) expected_elements = [ - f"-g {options.device_num}", - f"-t {os.cpu_count()}", - "--verbose 2", + "-g", + f"{options.device_num}", + "-t", + f"{os.cpu_count()}", + "--verbose", + "2", "--p_raw", "--u_max", - f"-s {self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor + "-s", + f"{self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor ] for element in expected_elements: self.assertIn(element, options_list) @@ -116,11 +120,14 @@ def test_as_list_windows(self): options_list = options.as_list(self.mock_sensor) expected_elements = [ - f"-g {options.device_num}", - "--verbose 2", + "-g", + f"{options.device_num}", + "--verbose", + "2", "--p_raw", "--u_max", - f"-s {self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor + "-s", + f"{self.mock_sensor.record_start_index}" ] for element in expected_elements: self.assertIn(element, options_list) @@ -136,12 +143,16 @@ def test_as_list_darwin(self): options_list = options.as_list(self.mock_sensor) expected_elements = [ - f"-g {options.device_num}", - f"-t {os.cpu_count()}", - "--verbose 2", + "-g", + f"{options.device_num}", + "-t", + f"{os.cpu_count()}", + "--verbose", + "2", "--p_raw", "--u_max", - f"-s {self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor + "-s", + f"{self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor ] for element in expected_elements: self.assertIn(element, options_list) @@ -156,9 +167,12 @@ def test_as_list_custom_record(self): options_list = options.as_list(self.mock_sensor) expected_elements = [ - f"-g {options.device_num}", - "-t 8", - "--verbose 1", + "-g", + f"{options.device_num}", + "-t", + "8", + "--verbose", + "1", "--p_max", "--u_min", "--p_raw" # Default if no specific 'p' or 'u' options are given @@ -183,8 +197,10 @@ def test_as_list_no_record(self): options_list = options.as_list(self.mock_sensor) expected_elements = [ - f"-g {options.device_num}", - "-t 4", + "-g", + f"{options.device_num}", + "-t", + "4", "--p_raw", # Default value ] for element in expected_elements: From 99cc1fb77c17423e562afd248f2eb99c100f6211 Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 14:52:44 -0800 Subject: [PATCH 07/11] remove explicit number of threads --- tests/test_simulation_execution_options.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 8a25de9b..7e2a9f3c 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -162,7 +162,7 @@ def test_as_list_custom_record(self): options = self.default_options self.mock_sensor.record = ["p_max", "u_min", "I_avg"] options.device_num = 2 - options.num_threads = 8 + options.num_threads = os.cpu_count() options.verbose_level = 1 options_list = options.as_list(self.mock_sensor) @@ -192,7 +192,7 @@ def test_as_list_no_record(self): options = self.default_options self.mock_sensor.record = None options.device_num = 1 - options.num_threads = 4 + options.num_threads = os.cpu_count() options.verbose_level = 0 options_list = options.as_list(self.mock_sensor) @@ -210,7 +210,7 @@ def test_list_compared_to_string(self): """Test the list representation compared to the string representation.""" options = self.default_options options.device_num = 1 - options.num_threads = 4 + options.num_threads = os.cpu_count() options.verbose_level = 1 options_list = options.as_list(self.mock_sensor) From 61d1901fbc2aa40e9fef09c35ff3e16fa94bdc1a Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 15:12:01 -0800 Subject: [PATCH 08/11] extend coverage for negative device number --- kwave/options/simulation_execution_options.py | 20 +++++++++++++------ tests/test_simulation_execution_options.py | 15 +++++++++----- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/kwave/options/simulation_execution_options.py b/kwave/options/simulation_execution_options.py index 07d1e9b2..f29834fc 100644 --- a/kwave/options/simulation_execution_options.py +++ b/kwave/options/simulation_execution_options.py @@ -34,8 +34,8 @@ def __init__( self._binary_dir = binary_dir self.kwave_function_name = kwave_function_name self.delete_data = delete_data - self.device_num = device_num - self.num_threads = num_threads + self._device_num = device_num + self._num_threads = num_threads self.thread_binding = thread_binding self.system_call = system_call self.verbose_level = verbose_level @@ -149,19 +149,27 @@ def binary_dir(self, value: str): f"{value} is not a directory. If you are trying to set the `binary_path`, use the `binary_path` attribute instead." ) self._binary_dir = Path(value) + + @property + def device_num(self) -> Optional[int]: + return self._device_num + + @device_num.setter + def device_num(self, value: Optional[int]): + if value is not None and value < 0: + raise ValueError("Device number must be non-negative") + self._device_num = value def as_list(self, sensor: kSensor) -> list[str]: options_list = [] if self.device_num is not None: - if self.device_num < 0: - raise ValueError("Device number must be non-negative") options_list.append("-g") options_list.append(str(self.device_num)) - if self.num_threads is not None and PLATFORM != "windows": + if self._num_threads is not None and PLATFORM != "windows": options_list.append("-t") - options_list.append(str(self.num_threads)) + options_list.append(str(self._num_threads)) if self.verbose_level > 0: options_list.append("--verbose") diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 7e2a9f3c..5542790c 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -26,7 +26,7 @@ def test_default_initialization(self): self.assertEqual(options.kwave_function_name, "kspaceFirstOrder3D") self.assertTrue(options.delete_data) self.assertIsNone(options.device_num) - self.assertEqual(options.num_threads, os.cpu_count()) # "all" should default to CPU count + self.assertEqual(options.num_threads, "all") # "all" should default to CPU count self.assertIsNone(options.thread_binding) self.assertEqual(options.verbose_level, 0) self.assertTrue(options.auto_chunking) @@ -75,6 +75,11 @@ def test_is_gpu_simulation_setter(self): self.assertFalse(options.is_gpu_simulation) self.assertEqual(options.binary_name, OMP_BINARY_NAME) + def test_device_num_setter_invalid(self): + """Test setting an invalid device number.""" + options = self.default_options + + def test_binary_name_custom(self): """Test setting a custom binary name.""" options = self.default_options @@ -170,7 +175,7 @@ def test_as_list_custom_record(self): "-g", f"{options.device_num}", "-t", - "8", + f"{os.cpu_count()}", "--verbose", "1", "--p_max", @@ -183,9 +188,9 @@ def test_as_list_custom_record(self): def test_as_list_with_invalid_values(self): """Test the behavior of as_list when there are invalid values.""" options = self.default_options - options.device_num = -1 # Invalid device_num should raise an exception when set with self.assertRaises(ValueError): - options.as_list(self.mock_sensor) + options.device_num = -1 + def test_as_list_no_record(self): """Test the list representation when there is no record.""" @@ -200,7 +205,7 @@ def test_as_list_no_record(self): "-g", f"{options.device_num}", "-t", - "4", + f"{os.cpu_count()}", "--p_raw", # Default value ] for element in expected_elements: From 80203299cc7f5a39040255731b813bf5e4aec50b Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 15:33:53 -0800 Subject: [PATCH 09/11] add windows exclusion for expected values --- tests/test_simulation_execution_options.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 5542790c..1d84ea26 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -136,7 +136,8 @@ def test_as_list_windows(self): ] for element in expected_elements: self.assertIn(element, options_list) - self.assertNotIn(f"-t {os.cpu_count()}", options_list) + self.assertNotIn("-t", options_list) + self.assertNotIn(f"{os.cpu_count()}", options_list) @patch("kwave.options.simulation_execution_options.PLATFORM", "darwin") def test_as_list_darwin(self): @@ -174,14 +175,17 @@ def test_as_list_custom_record(self): expected_elements = [ "-g", f"{options.device_num}", - "-t", - f"{os.cpu_count()}", + "--verbose", "1", "--p_max", "--u_min", "--p_raw" # Default if no specific 'p' or 'u' options are given ] + if not PLATFORM == "windows": + expected_elements.append("-t") + expected_elements.append(f"{os.cpu_count()}") + for element in expected_elements: self.assertIn(element, options_list) @@ -204,10 +208,13 @@ def test_as_list_no_record(self): expected_elements = [ "-g", f"{options.device_num}", - "-t", - f"{os.cpu_count()}", + "--p_raw", # Default value ] + + if not PLATFORM == "windows": + expected_elements.append("-t") + expected_elements.append(f"{os.cpu_count()}") for element in expected_elements: self.assertIn(element, options_list) From 493021cebd59178d61d73283b1ca97dd6d06bd2e Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 18:53:12 -0800 Subject: [PATCH 10/11] update "all" default value --- kwave/options/simulation_execution_options.py | 22 ++++++++++--------- tests/test_simulation_execution_options.py | 2 +- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/kwave/options/simulation_execution_options.py b/kwave/options/simulation_execution_options.py index f29834fc..2e82a49b 100644 --- a/kwave/options/simulation_execution_options.py +++ b/kwave/options/simulation_execution_options.py @@ -21,7 +21,7 @@ def __init__( kwave_function_name: Optional[str] = "kspaceFirstOrder3D", delete_data: bool = True, device_num: Optional[int] = None, - num_threads: Union[int, str] = "all", + num_threads: Optional[int] = None, thread_binding: Optional[bool] = None, system_call: Optional[str] = None, verbose_level: int = 0, @@ -34,8 +34,8 @@ def __init__( self._binary_dir = binary_dir self.kwave_function_name = kwave_function_name self.delete_data = delete_data - self._device_num = device_num - self._num_threads = num_threads + self.device_num = device_num + self.num_threads = num_threads self.thread_binding = thread_binding self.system_call = system_call self.verbose_level = verbose_level @@ -53,7 +53,11 @@ def num_threads(self, value: Union[int, str]): raise RuntimeError("Unable to determine the number of CPUs on this system. Please specify the number of threads explicitly.") if value == "all": - value = cpu_count + warnings.warn("The 'all' option is deprecated. The value of None sets the maximal number of threads (excluding Windows).", DeprecationWarning) + value = cpu_count + + if value is None: + value = cpu_count if not isinstance(value, int): raise ValueError("Got {value}. Number of threads must be 'all' or a positive integer") @@ -87,10 +91,8 @@ def is_gpu_simulation(self, value: Optional[bool]): @property def binary_name(self) -> str: - valid_binary_names = ["kspaceFirstOrder-CUDA", "kspaceFirstOrder-OMP"] - if PLATFORM == "windows": - valid_binary_names = [name + ".exe" for name in valid_binary_names] - + + valid_binary_names = ["kspaceFirstOrder-OMP", "kspaceFirstOrder-CUDA"] if self._binary_name is None: # set default binary name based on GPU simulation value if self.is_gpu_simulation is None: @@ -103,9 +105,9 @@ def binary_name(self) -> str: if PLATFORM == "windows": self._binary_name += ".exe" + valid_binary_names = [name + ".exe" for name in valid_binary_names] + elif self._binary_name not in valid_binary_names: - import warnings - warnings.warn("Custom binary name set. Ignoring `is_gpu_simulation` state.") return self._binary_name diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 1d84ea26..f54d4028 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -26,7 +26,7 @@ def test_default_initialization(self): self.assertEqual(options.kwave_function_name, "kspaceFirstOrder3D") self.assertTrue(options.delete_data) self.assertIsNone(options.device_num) - self.assertEqual(options.num_threads, "all") # "all" should default to CPU count + self.assertEqual(options._num_threads, os.cpu_count()) self.assertIsNone(options.thread_binding) self.assertEqual(options.verbose_level, 0) self.assertTrue(options.auto_chunking) From 0205e082a3339e5b9c19c9d7740ec998690e86e6 Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 14 Dec 2024 19:59:27 -0800 Subject: [PATCH 11/11] test list equality --- kwave/options/simulation_execution_options.py | 2 +- tests/test_simulation_execution_options.py | 59 +++++++++---------- 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/kwave/options/simulation_execution_options.py b/kwave/options/simulation_execution_options.py index 2e82a49b..994858d9 100644 --- a/kwave/options/simulation_execution_options.py +++ b/kwave/options/simulation_execution_options.py @@ -186,7 +186,7 @@ def as_list(self, sensor: kSensor) -> list[str]: } if sensor.record is not None: - matching_keys = set(sensor.record).intersection(record_options_map.keys()) + matching_keys = sorted(set(sensor.record).intersection(record_options_map.keys())) options_list.extend([f"--{record_options_map[key]}" for key in matching_keys]) if "u_non_staggered" in sensor.record or "I_avg" in sensor.record or "I" in sensor.record: diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index f54d4028..b39e774f 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -96,15 +96,15 @@ def test_get_options_string_darwin(self): """Test the get_options_string method with a mock sensor.""" options = self.default_options options.device_num = 1 - options.num_threads = os.cpu_count() + options.num_threads = 1 options.verbose_level = 2 options_list = options.as_list(self.mock_sensor) expected_elements = [ "-g", - f"{options.device_num}", + "1", "-t", - f"{os.cpu_count()}", + "1", "--verbose", "2", "--p_raw", @@ -112,21 +112,20 @@ def test_get_options_string_darwin(self): "-s", f"{self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor ] - for element in expected_elements: - self.assertIn(element, options_list) + self.assertListEqual(expected_elements, options_list) @patch("kwave.options.simulation_execution_options.PLATFORM", "windows") def test_as_list_windows(self): """Test the list representation of options on Windows.""" options = self.default_options options.device_num = 1 - options.num_threads = os.cpu_count() + options.num_threads = 1 options.verbose_level = 2 options_list = options.as_list(self.mock_sensor) expected_elements = [ "-g", - f"{options.device_num}", + "1", "--verbose", "2", "--p_raw", @@ -134,17 +133,14 @@ def test_as_list_windows(self): "-s", f"{self.mock_sensor.record_start_index}" ] - for element in expected_elements: - self.assertIn(element, options_list) - self.assertNotIn("-t", options_list) - self.assertNotIn(f"{os.cpu_count()}", options_list) + self.assertListEqual(expected_elements, options_list) @patch("kwave.options.simulation_execution_options.PLATFORM", "darwin") def test_as_list_darwin(self): """Test the list representation of options on macOS.""" options = self.default_options options.device_num = 1 - options.num_threads = os.cpu_count() + options.num_threads = 1 options.verbose_level = 2 options_list = options.as_list(self.mock_sensor) @@ -152,7 +148,7 @@ def test_as_list_darwin(self): "-g", f"{options.device_num}", "-t", - f"{os.cpu_count()}", + f"1", "--verbose", "2", "--p_raw", @@ -160,34 +156,34 @@ def test_as_list_darwin(self): "-s", f"{self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor ] - for element in expected_elements: - self.assertIn(element, options_list) + + self.assertListEqual(expected_elements, options_list) def test_as_list_custom_record(self): """Test the list representation with a custom record configuration.""" options = self.default_options self.mock_sensor.record = ["p_max", "u_min", "I_avg"] - options.device_num = 2 - options.num_threads = os.cpu_count() + options.device_num = 1 + options.num_threads = 1 options.verbose_level = 1 options_list = options.as_list(self.mock_sensor) expected_elements = [ "-g", - f"{options.device_num}", - + "1", "--verbose", "1", "--p_max", "--u_min", - "--p_raw" # Default if no specific 'p' or 'u' options are given + '--u_non_staggered_raw', + "--p_raw", + '-s', + '10', ] if not PLATFORM == "windows": - expected_elements.append("-t") - expected_elements.append(f"{os.cpu_count()}") - - for element in expected_elements: - self.assertIn(element, options_list) + expected_elements.insert(2,"-t") + expected_elements.insert(3,f"1") + self.assertListEqual(expected_elements, options_list) def test_as_list_with_invalid_values(self): """Test the behavior of as_list when there are invalid values.""" @@ -201,22 +197,23 @@ def test_as_list_no_record(self): options = self.default_options self.mock_sensor.record = None options.device_num = 1 - options.num_threads = os.cpu_count() + options.num_threads = 1 options.verbose_level = 0 options_list = options.as_list(self.mock_sensor) expected_elements = [ "-g", f"{options.device_num}", - "--p_raw", # Default value + "-s", # start timestep index + "10" ] if not PLATFORM == "windows": - expected_elements.append("-t") - expected_elements.append(f"{os.cpu_count()}") - for element in expected_elements: - self.assertIn(element, options_list) + expected_elements.insert(2, "-t") + expected_elements.insert(3, "1") + self.assertListEqual(expected_elements, options_list) + def test_list_compared_to_string(self): """Test the list representation compared to the string representation."""