From 528c1bfba765907e510478148d38ed80569a824b Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 24 May 2025 09:27:50 -0700 Subject: [PATCH 1/9] Improve checkpointing parameter validation --- kwave/options/simulation_execution_options.py | 19 +++- tests/test_simulation_execution_options.py | 103 +++++++++++------- 2 files changed, 81 insertions(+), 41 deletions(-) diff --git a/kwave/options/simulation_execution_options.py b/kwave/options/simulation_execution_options.py index f7bbf7c2..163e60b7 100644 --- a/kwave/options/simulation_execution_options.py +++ b/kwave/options/simulation_execution_options.py @@ -51,9 +51,20 @@ def __init__( self.checkpoint_timesteps = checkpoint_timesteps self.checkpoint_file = checkpoint_file - if self.checkpoint_file is not None: - if self.checkpoint_interval is None and self.checkpoint_timesteps is None: - raise ValueError("One of checkpoint_interval or checkpoint_timesteps must be set when checkpoint_file is set.") + self._validate_checkpoint_options() + + def _validate_checkpoint_options(self): + # Checkpointing parameters are set + if self.checkpoint_interval is not None or self.checkpoint_timesteps is not None: + # No checkpoint file set + if self.checkpoint_file is None: + raise ValueError("`checkpoint_file` must be set when `checkpoint_interval` or `checkpoint_timesteps` is set.") + # Both checkpointing parameters are set + if self.checkpoint_interval is not None and self.checkpoint_timesteps is not None: + raise ValueError("`checkpoint_interval` and `checkpoint_timesteps` cannot be set at the same time.") + # Checkpoint file is set but no checkpointing parameters are set + if self.checkpoint_file is not None and self.checkpoint_interval is None and self.checkpoint_timesteps is None: + raise ValueError("`checkpoint_interval` or `checkpoint_timesteps` must be set when `checkpoint_file` is set.") @property def num_threads(self) -> Union[int, str]: @@ -185,7 +196,7 @@ def checkpoint_interval(self) -> Optional[int]: def checkpoint_interval(self, value: Optional[int]): if value is not None: if not isinstance(value, int) or value < 0: - raise ValueError("Checkpoint interval must be a positive integer") + raise ValueError("Checkpoint interval must be a positive integer in seconds") self._checkpoint_interval = value @property diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 0af0c330..d2ab4634 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -266,43 +266,20 @@ def test_as_list_with_valid_checkpoint_timesteps_options(self): def test_initialization_with_invalid_checkpoint_options(self): """Test initialization with invalid checkpoint options.""" - - # Test with valid checkpoint_file but unset checkpoint_timesteps and checkpoint_interval with TemporaryDirectory() as temp_dir: checkpoint_file = Path(temp_dir) / "checkpoint.h5" - with self.assertRaises(ValueError): - SimulationExecutionOptions(checkpoint_file=checkpoint_file) - - # Test with invalid checkpoint_file type - with self.assertRaises(ValueError): - SimulationExecutionOptions(checkpoint_file=12345, checkpoint_timesteps=10) - - # Test with invalid checkpoint_timesteps - with self.assertRaises(ValueError): - SimulationExecutionOptions(checkpoint_timesteps=-1, checkpoint_file="checkpoint.h5") - - with self.assertRaises(ValueError): - SimulationExecutionOptions(checkpoint_timesteps="not_an_integer", checkpoint_file="checkpoint.h5") - - # Test with invalid checkpoint_interval - with self.assertRaises(ValueError): - SimulationExecutionOptions(checkpoint_interval=-1, checkpoint_file="checkpoint.h5") - with self.assertRaises(ValueError): - SimulationExecutionOptions(checkpoint_interval="not_an_integer", checkpoint_file="checkpoint.h5") + # Test with invalid checkpoint_file type + with self.assertRaises(ValueError): + SimulationExecutionOptions(checkpoint_file=12345, checkpoint_timesteps=10) - # Test with invalid checkpoint_file - with self.assertRaises(ValueError): - SimulationExecutionOptions( - checkpoint_interval=10, - checkpoint_file="checkpoint.txt", # Wrong extension - ) + # Test with both checkpoint options - should raise ValueError + with self.assertRaises(ValueError): + SimulationExecutionOptions(checkpoint_timesteps=10, checkpoint_interval=20, checkpoint_file=checkpoint_file) - with self.assertRaises(FileNotFoundError): - SimulationExecutionOptions( - checkpoint_interval=10, - checkpoint_file="nonexistent_dir/checkpoint.h5", # Non-existent directory - ) + # Test with just checkpoint_file (should raise ValueError) + with self.assertRaises(ValueError): + SimulationExecutionOptions(checkpoint_file=checkpoint_file) def test_initialization_with_valid_checkpoint_options(self): """Test initialization with valid checkpoint options.""" @@ -319,11 +296,63 @@ def test_initialization_with_valid_checkpoint_options(self): self.assertEqual(options.checkpoint_interval, 20) self.assertEqual(options.checkpoint_file, checkpoint_file) - # Test with both checkpoint options - should be valid - options = SimulationExecutionOptions(checkpoint_timesteps=10, checkpoint_interval=20, checkpoint_file=checkpoint_file) - self.assertEqual(options.checkpoint_timesteps, 10) - self.assertEqual(options.checkpoint_interval, 20) - self.assertEqual(options.checkpoint_file, checkpoint_file) + def test_checkpoint_interval_validation(self): + """Test validation of checkpoint_interval property.""" + options = self.default_options + + # Test valid values + options.checkpoint_interval = 0 + self.assertEqual(options.checkpoint_interval, 0) + options.checkpoint_interval = 100 + self.assertEqual(options.checkpoint_interval, 100) + + # Test invalid values + with self.assertRaises(ValueError): + options.checkpoint_interval = -1 + with self.assertRaises(ValueError): + options.checkpoint_interval = "invalid" + with self.assertRaises(ValueError): + options.checkpoint_interval = 1.5 + + def test_checkpoint_timesteps_validation(self): + """Test validation of checkpoint_timesteps property.""" + options = self.default_options + + # Test valid values + options.checkpoint_timesteps = 0 + self.assertEqual(options.checkpoint_timesteps, 0) + options.checkpoint_timesteps = 100 + self.assertEqual(options.checkpoint_timesteps, 100) + + # Test invalid values + with self.assertRaises(ValueError): + options.checkpoint_timesteps = -1 + with self.assertRaises(ValueError): + options.checkpoint_timesteps = "invalid" + with self.assertRaises(ValueError): + options.checkpoint_timesteps = 1.5 + + def test_checkpoint_file_validation(self): + """Test validation of checkpoint_file property.""" + options = self.default_options + + # Test valid values + with TemporaryDirectory() as temp_dir: + valid_path = Path(temp_dir) / "checkpoint.h5" + options.checkpoint_file = valid_path + self.assertEqual(options.checkpoint_file, valid_path) + + # Test string path + options.checkpoint_file = str(valid_path) + self.assertEqual(options.checkpoint_file, valid_path) + + # Test invalid values + with self.assertRaises(ValueError): + options.checkpoint_file = "invalid.extension" + with self.assertRaises(ValueError): + options.checkpoint_file = 123 + with self.assertRaises(FileNotFoundError): + options.checkpoint_file = "/nonexistent/path/checkpoint.h5" if __name__ == "__main__": From 5d0829a661d5cc95478530c22419f7990c6a3566 Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 24 May 2025 10:07:58 -0700 Subject: [PATCH 2/9] validate before simulating --- kwave/executor.py | 5 ++++- kwave/options/simulation_execution_options.py | 16 +++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/kwave/executor.py b/kwave/executor.py index 77e10803..0d66f056 100644 --- a/kwave/executor.py +++ b/kwave/executor.py @@ -33,6 +33,9 @@ def _make_binary_executable(self): binary_path.chmod(binary_path.stat().st_mode | stat.S_IEXEC) def run_simulation(self, input_filename: str, output_filename: str, options: list[str]) -> dotdict: + # Validate execution options before running simulation + self.execution_options.validate() + command = [str(self.execution_options.binary_path), "-i", input_filename, "-o", output_filename] + options try: @@ -116,7 +119,7 @@ def parse_executable_output(output_filename: str) -> dotdict: # # Combine the sensor data if using a kWaveTransducer as a sensor # if isinstance(sensor, kWaveTransducer): # sensor_data['p'] = sensor.combine_sensor_data(sensor_data['p']) - + # # # Compute the intensity outputs # if any(key.startswith(('I_avg', 'I')) for key in sensor.get('record', [])): # flags = { diff --git a/kwave/options/simulation_execution_options.py b/kwave/options/simulation_execution_options.py index 163e60b7..df9ce6a0 100644 --- a/kwave/options/simulation_execution_options.py +++ b/kwave/options/simulation_execution_options.py @@ -51,7 +51,7 @@ def __init__( self.checkpoint_timesteps = checkpoint_timesteps self.checkpoint_file = checkpoint_file - self._validate_checkpoint_options() + self.validate() def _validate_checkpoint_options(self): # Checkpointing parameters are set @@ -66,6 +66,20 @@ def _validate_checkpoint_options(self): if self.checkpoint_file is not None and self.checkpoint_interval is None and self.checkpoint_timesteps is None: raise ValueError("`checkpoint_interval` or `checkpoint_timesteps` must be set when `checkpoint_file` is set.") + def validate(self): + """Validate all simulation options before running a simulation. + + This method should be called before running a simulation to ensure all options + are in a valid state. It validates: + 1. Checkpoint configuration (if any checkpoint options are set) + + Raises: + ValueError: If any option configuration is invalid + """ + # Validate checkpoint configuration if any checkpoint options are set + if any(x is not None for x in [self.checkpoint_interval, self.checkpoint_timesteps, self.checkpoint_file]): + self._validate_checkpoint_options() + @property def num_threads(self) -> Union[int, str]: return self._num_threads From 1d47860b00d675298f285924ce05119e30ed7ce8 Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 24 May 2025 10:19:42 -0700 Subject: [PATCH 3/9] Simplify and extend tests --- tests/test_simulation_execution_options.py | 33 +++++++++++----------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index d2ab4634..85e4b154 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -333,26 +333,27 @@ def test_checkpoint_timesteps_validation(self): options.checkpoint_timesteps = 1.5 def test_checkpoint_file_validation(self): - """Test validation of checkpoint_file property.""" + """Test validation of checkpoint file path.""" options = self.default_options + with self.assertRaises(ValueError): + options.checkpoint_file = "invalid/path/checkpoint.h5" - # Test valid values - with TemporaryDirectory() as temp_dir: - valid_path = Path(temp_dir) / "checkpoint.h5" - options.checkpoint_file = valid_path - self.assertEqual(options.checkpoint_file, valid_path) + def test_checkpoint_file_required_when_parameters_set(self): + """Test that checkpoint file is required when checkpoint parameters are set.""" + options = self.default_options - # Test string path - options.checkpoint_file = str(valid_path) - self.assertEqual(options.checkpoint_file, valid_path) + # Test with checkpoint_interval + options.checkpoint_interval = 10 + with self.assertRaises(ValueError) as cm: + options.validate() + self.assertEqual(str(cm.exception), "`checkpoint_file` must be set when `checkpoint_interval` or `checkpoint_timesteps` is set.") - # Test invalid values - with self.assertRaises(ValueError): - options.checkpoint_file = "invalid.extension" - with self.assertRaises(ValueError): - options.checkpoint_file = 123 - with self.assertRaises(FileNotFoundError): - options.checkpoint_file = "/nonexistent/path/checkpoint.h5" + # Test with checkpoint_timesteps + options.checkpoint_interval = None + options.checkpoint_timesteps = 10 + with self.assertRaises(ValueError) as cm: + options.validate() + self.assertEqual(str(cm.exception), "`checkpoint_file` must be set when `checkpoint_interval` or `checkpoint_timesteps` is set.") if __name__ == "__main__": From 17958d9e64a3f8daa8588f2033abfdc580649fec Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 24 May 2025 10:20:52 -0700 Subject: [PATCH 4/9] minor fix Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- kwave/executor.py | 1 - 1 file changed, 1 deletion(-) diff --git a/kwave/executor.py b/kwave/executor.py index 0d66f056..7caff892 100644 --- a/kwave/executor.py +++ b/kwave/executor.py @@ -119,7 +119,6 @@ def parse_executable_output(output_filename: str) -> dotdict: # # Combine the sensor data if using a kWaveTransducer as a sensor # if isinstance(sensor, kWaveTransducer): # sensor_data['p'] = sensor.combine_sensor_data(sensor_data['p']) - # # # Compute the intensity outputs # if any(key.startswith(('I_avg', 'I')) for key in sensor.get('record', [])): # flags = { From 747fe3b3b4a180aa96c75499cd5a8a8df3172dfe Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 24 May 2025 20:46:19 -0700 Subject: [PATCH 5/9] Correct for file not found --- tests/test_simulation_execution_options.py | 23 +++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 85e4b154..329054d8 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -335,8 +335,29 @@ def test_checkpoint_timesteps_validation(self): def test_checkpoint_file_validation(self): """Test validation of checkpoint file path.""" options = self.default_options - with self.assertRaises(ValueError): + + # Test with non-existent directory + with self.assertRaises(FileNotFoundError) as cm: options.checkpoint_file = "invalid/path/checkpoint.h5" + self.assertEqual(str(cm.exception), "Checkpoint folder invalid/path does not exist.") + + # Test with temporary directory + with TemporaryDirectory() as temp_dir: + # Test invalid file extension + invalid_file = Path(temp_dir) / "checkpoint.txt" + with self.assertRaises(ValueError) as cm: + options.checkpoint_file = invalid_file + self.assertEqual(str(cm.exception), f"Checkpoint file {invalid_file} must have .h5 extension.") + + # Test valid file path + valid_file = Path(temp_dir) / "checkpoint.h5" + options.checkpoint_file = valid_file + self.assertEqual(options.checkpoint_file, valid_file) + + # Test invalid type + with self.assertRaises(ValueError) as cm: + options.checkpoint_file = 123 + self.assertEqual(str(cm.exception), "Checkpoint file must be a string or Path object.") def test_checkpoint_file_required_when_parameters_set(self): """Test that checkpoint file is required when checkpoint parameters are set.""" From 125e3000a4af927c500826c9cd2d1801afbbc131 Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 24 May 2025 21:06:15 -0700 Subject: [PATCH 6/9] Update error message on windows. --- tests/test_simulation_execution_options.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index 329054d8..a7558e60 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -339,7 +339,8 @@ def test_checkpoint_file_validation(self): # Test with non-existent directory with self.assertRaises(FileNotFoundError) as cm: options.checkpoint_file = "invalid/path/checkpoint.h5" - self.assertEqual(str(cm.exception), "Checkpoint folder invalid/path does not exist.") + expected_folder = str(Path("invalid") / "path") + self.assertEqual(str(cm.exception), f"Checkpoint folder {expected_folder} does not exist.") # Test with temporary directory with TemporaryDirectory() as temp_dir: From f34b2c9e7ee5931704329d5f468d0ab7755cb529 Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 24 May 2025 21:21:34 -0700 Subject: [PATCH 7/9] Fix validation --- kwave/options/simulation_execution_options.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kwave/options/simulation_execution_options.py b/kwave/options/simulation_execution_options.py index df9ce6a0..8e699cfa 100644 --- a/kwave/options/simulation_execution_options.py +++ b/kwave/options/simulation_execution_options.py @@ -209,7 +209,7 @@ def checkpoint_interval(self) -> Optional[int]: @checkpoint_interval.setter def checkpoint_interval(self, value: Optional[int]): if value is not None: - if not isinstance(value, int) or value < 0: + if not isinstance(value, int) or value <= 0: raise ValueError("Checkpoint interval must be a positive integer in seconds") self._checkpoint_interval = value From fe2e7a957484c2d3e6228eda43405a7606dac37c Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 24 May 2025 21:30:48 -0700 Subject: [PATCH 8/9] Revert changes --- kwave/options/simulation_execution_options.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kwave/options/simulation_execution_options.py b/kwave/options/simulation_execution_options.py index 8e699cfa..df9ce6a0 100644 --- a/kwave/options/simulation_execution_options.py +++ b/kwave/options/simulation_execution_options.py @@ -209,7 +209,7 @@ def checkpoint_interval(self) -> Optional[int]: @checkpoint_interval.setter def checkpoint_interval(self, value: Optional[int]): if value is not None: - if not isinstance(value, int) or value <= 0: + if not isinstance(value, int) or value < 0: raise ValueError("Checkpoint interval must be a positive integer in seconds") self._checkpoint_interval = value From 005d8b202643a0f315cd60dfdf954a5e5a81916a Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 24 May 2025 21:58:38 -0700 Subject: [PATCH 9/9] Switch to positive values and update tests --- kwave/options/simulation_execution_options.py | 2 +- tests/test_simulation_execution_options.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/kwave/options/simulation_execution_options.py b/kwave/options/simulation_execution_options.py index df9ce6a0..8e699cfa 100644 --- a/kwave/options/simulation_execution_options.py +++ b/kwave/options/simulation_execution_options.py @@ -209,7 +209,7 @@ def checkpoint_interval(self) -> Optional[int]: @checkpoint_interval.setter def checkpoint_interval(self, value: Optional[int]): if value is not None: - if not isinstance(value, int) or value < 0: + if not isinstance(value, int) or value <= 0: raise ValueError("Checkpoint interval must be a positive integer in seconds") self._checkpoint_interval = value diff --git a/tests/test_simulation_execution_options.py b/tests/test_simulation_execution_options.py index a7558e60..04da4890 100644 --- a/tests/test_simulation_execution_options.py +++ b/tests/test_simulation_execution_options.py @@ -301,12 +301,12 @@ def test_checkpoint_interval_validation(self): options = self.default_options # Test valid values - options.checkpoint_interval = 0 - self.assertEqual(options.checkpoint_interval, 0) options.checkpoint_interval = 100 self.assertEqual(options.checkpoint_interval, 100) # Test invalid values + with self.assertRaises(ValueError): + options.checkpoint_interval = 0 with self.assertRaises(ValueError): options.checkpoint_interval = -1 with self.assertRaises(ValueError):