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..994858d9 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 @@ -20,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, @@ -52,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") @@ -86,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: @@ -102,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 @@ -148,52 +151,63 @@ 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 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}") + 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}") + if self._num_threads is not None and PLATFORM != "windows": + 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", - "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]}") + 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: - 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("-s") + options_list.append(f"{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) 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" 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() diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 1646bcce..b39e774f 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, os.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 @@ -91,78 +96,135 @@ 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_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 = [ + "-g", + "1", + "-t", + "1", + "--verbose", + "2", + "--p_raw", + "--u_max", + "-s", + f"{self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor + ] + self.assertListEqual(expected_elements, 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.num_threads = 1 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 = [ + "-g", + "1", + "--verbose", + "2", + "--p_raw", + "--u_max", + "-s", + f"{self.mock_sensor.record_start_index}" + ] + self.assertListEqual(expected_elements, 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.num_threads = 1 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_list = options.as_list(self.mock_sensor) + expected_elements = [ + "-g", + f"{options.device_num}", + "-t", + f"1", + "--verbose", + "2", + "--p_raw", + "--u_max", + "-s", + f"{self.mock_sensor.record_start_index}" # Updated to use self.mock_sensor + ] + + 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 = 1 + options.num_threads = 1 + options.verbose_level = 1 + + options_list = options.as_list(self.mock_sensor) + expected_elements = [ + "-g", + "1", + "--verbose", + "1", + "--p_max", + "--u_min", + '--u_non_staggered_raw', + "--p_raw", + '-s', + '10', + ] + if not PLATFORM == "windows": + 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.""" + options = self.default_options + with self.assertRaises(ValueError): + options.device_num = -1 - options.is_gpu_simulation = False - self.assertEqual(options.binary_name, OMP_BINARY_NAME) - self.assertTrue(str(options.binary_path).endswith(OMP_BINARY_NAME)) - 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 = 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.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.""" + options = self.default_options + options.device_num = 1 + options.num_threads = os.cpu_count() + 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()