Update execution options APIs

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(

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

kwave/kspaceFirstOrder3D.py

@@ -461,6 +461,6 @@
             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

kwave/kspaceFirstOrderAS.py

@@ -365,6 +365,6 @@
             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

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
@@ -33,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
@@ -148,52 +149,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]}")
+            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)
 

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"

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()

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
@@ -94,75 +99,135 @@ 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 = [
+            "-g",
+            f"{options.device_num}",
+            "-t",
+            f"{os.cpu_count()}",
+            "--verbose",
+            "2",
+            "--p_raw",
+            "--u_max",
+            "-s",
+            f"{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 = [
+            "-g",
+            f"{options.device_num}",
+            "--verbose",
+            "2",
+            "--p_raw",
+            "--u_max",
+            "-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)
 
-    @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)
+        options_list = options.as_list(self.mock_sensor)
+        expected_elements = [
+            "-g",
+            f"{options.device_num}",
+            "-t",
+            f"{os.cpu_count()}",
+            "--verbose",
+            "2",
+            "--p_raw",
+            "--u_max",
+            "-s",
+            f"{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 = os.cpu_count()
+        options.verbose_level = 1
+
+        options_list = options.as_list(self.mock_sensor)
+        expected_elements = [
+            "-g",
+            f"{options.device_num}",
+
+            "--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)
+
+    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
 
-    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))
+    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 = os.cpu_count()
+        options.verbose_level = 0
 
-    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)
+        options_list = options.as_list(self.mock_sensor)
+        expected_elements = [
+            "-g",
+            f"{options.device_num}",
 
+            "--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)
+
+    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()