Improve checkpointing parameter validation #611

waltsims · 2025-05-24T16:29:17Z

This pull request refactors and enhances the validation logic for checkpoint-related options in the SimulationExecutionOptions class and improves the corresponding test coverage. The most important changes include extracting validation logic into a dedicated method, updating error messages, and adding comprehensive unit tests for validation.

Refactoring and Validation Enhancements:

Extracted checkpoint validation logic into a new _validate_checkpoint_options method in SimulationExecutionOptions, improving code readability and maintainability. The method enforces rules such as requiring checkpoint_file when checkpoint_interval or checkpoint_timesteps is set and ensuring that both checkpoint_interval and checkpoint_timesteps cannot be set simultaneously.
Updated the error message for invalid checkpoint_interval to clarify that the value must be a positive integer in seconds.

Test Suite Improvements:

Refactored and expanded test cases for invalid checkpoint options, consolidating and simplifying the test logic. Tests now include cases for scenarios such as setting both checkpoint options, missing checkpoint_file, and invalid parameter types.
Added new dedicated test methods for validating individual properties (checkpoint_interval, checkpoint_timesteps, and checkpoint_file) with both valid and invalid inputs. These tests ensure stricter validation of input types and values.

Summary by CodeRabbit

New Features
- Pre-run validation of simulation execution options to catch misconfiguration early.
Bug Fixes
- Stricter checkpoint configuration rules:
  - Providing interval or timesteps now requires a checkpoint file.
  - Interval and timesteps can no longer be set together.
  - Interval must be a positive integer; timesteps must be a non-negative integer.
- Clearer, more precise error messages for invalid checkpoint paths and extensions.
Tests
- Expanded test coverage for checkpoint options, including value validation, path checks, and invalid option combinations.

Copilot

Pull Request Overview

Refactors checkpoint parameter validation in SimulationExecutionOptions into a dedicated method, clarifies error messages, and adds granular unit tests for each checkpoint property.

Extracted and consolidated checkpoint-option validation into _validate_checkpoint_options.
Updated the error message for checkpoint_interval to mention units, and added setter–level tests for all checkpoint properties.
Expanded test coverage with dedicated validation tests for checkpoint_interval, checkpoint_timesteps, and checkpoint_file.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
tests/test_simulation_execution_options.py	Streamlined invalid-init tests and added per-property validation tests
kwave/options/simulation_execution_options.py	Added `_validate_checkpoint_options`, updated error messages in checkpoint setters

Comments suppressed due to low confidence (3)

kwave/options/simulation_execution_options.py:207

0 is accepted in tests but the message says "positive integer"; update to "non-negative integer" for consistency.

raise ValueError("Checkpoint timesteps must be a positive integer")

tests/test_simulation_execution_options.py:314

Add a test case that sets checkpoint_interval and then checkpoint_timesteps (or vice versa) on an existing options instance to verify that conflicts are caught when properties change after initialization.

with self.assertRaises(ValueError):

kwave/options/simulation_execution_options.py:1

[nitpick] Consider updating the class docstring to briefly describe the exclusive relationship between checkpoint_interval and checkpoint_timesteps, and the requirement for checkpoint_file.

class SimulationExecutionOptions:

kwave/options/simulation_execution_options.py

codecov · 2025-05-24T16:31:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.99%. Comparing base (96441a4) to head (0cde11d).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #611      +/-   ##
==========================================
+ Coverage   73.95%   73.99%   +0.03%     
==========================================
  Files          50       50              
  Lines        7000     7010      +10     
  Branches     1338     1341       +3     
==========================================
+ Hits         5177     5187      +10     
  Misses       1280     1280              
  Partials      543      543

Flag	Coverage Δ
3.10	`73.99% <100.00%> (+0.03%)`	⬆️
3.11	`73.99% <100.00%> (+0.03%)`	⬆️
3.12	`73.99% <100.00%> (+0.03%)`	⬆️
3.13	`73.99% <100.00%> (+0.03%)`	⬆️
macos-latest	`73.96% <100.00%> (+0.03%)`	⬆️
ubuntu-latest	`73.96% <100.00%> (+0.03%)`	⬆️
windows-latest	`73.98% <100.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

This PR centralizes and improves checkpoint-related validation logic in SimulationExecutionOptions, updates corresponding error messages, and enhances test coverage for checkpoint parameters.

Extract and encapsulate checkpoint validations into a new _validate_checkpoint_options method and a public validate() call.
Update the checkpoint_interval error message to specify “in seconds” and invoke validation in the executor before running simulations.
Refactor and expand the unit test suite with dedicated tests for checkpoint_interval, checkpoint_timesteps, and checkpoint_file validations.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
tests/test_simulation_execution_options.py	Streamlined invalid initialization tests and added separate validation tests per checkpoint field
kwave/options/simulation_execution_options.py	Added `_validate_checkpoint_options` and `validate()`, refined error messages for checkpoints
kwave/executor.py	Inserted a pre-run `validate()` call and introduced an extraneous placeholder comment

Comments suppressed due to low confidence (3)

kwave/executor.py:35

[nitpick] The parameter name 'options' may be confused with 'self.execution_options'; consider renaming it to 'cli_options' or similar to avoid ambiguity.

def run_simulation(self, input_filename: str, output_filename: str, options: list[str]) -> dotdict:

tests/test_simulation_execution_options.py:310

Indentation here does not align with the enclosing with TemporaryDirectory() block; adjust it to ensure this assertion runs inside the context.

            with self.assertRaises(ValueError):

tests/test_simulation_execution_options.py:310

Similarly, this assertion must be indented to match the other tests inside the TemporaryDirectory scope to avoid scope errors.

            with self.assertRaises(ValueError):

kwave/executor.py

Co-authored-by: Copilot <[email protected]>

Copilot

Pull Request Overview

This PR centralizes and strengthens checkpoint option validation by extracting logic into _validate_checkpoint_options, clarifies error messaging, and boosts test coverage with focused property tests.

Extracted checkpoint validation into SimulationExecutionOptions._validate_checkpoint_options and hooked it into a new validate() method
Updated the checkpoint_interval error message to specify “in seconds”
Added a pre-run validation call in run_simulation and expanded unit tests for each checkpoint property

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
tests/test_simulation_execution_options.py	Simplified the invalid checkpoint initialization test and added targeted property validation tests
kwave/options/simulation_execution_options.py	Moved checkpoint rules into `_validate_checkpoint_options`, updated error text, and introduced `validate()`
kwave/executor.py	Inserted a call to `self.execution_options.validate()` before simulation execution

Comments suppressed due to low confidence (2)

kwave/executor.py:35

[nitpick] The parameter name options can be confused with self.execution_options; consider renaming it to something like cli_options to avoid shadowing and improve readability.

def run_simulation(self, input_filename: str, output_filename: str, options: list[str]) -> dotdict:

tests/test_simulation_execution_options.py:276

[nitpick] The indentation of comments and with blocks in this test are inconsistent, which can make it harder to read; aligning them will improve clarity.

# Test with both checkpoint options - should raise ValueError

Copilot · 2025-05-25T04:32:38Z

kwave/options/simulation_execution_options.py

+                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.")


[nitpick] The backticks in the exception message are literal characters; for consistency with other validators you may want to remove them or switch to quotes.

Suggested change

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.")

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.")

Copilot

Pull Request Overview

This pull request refactors checkpoint parameter validation in SimulationExecutionOptions and extends the test coverage to better enforce valid configurations.

Extracts checkpoint validation logic into a dedicated _validate_checkpoint_options method
Updates error messages to clarify parameter requirements
Enhances and adds new unit tests covering checkpoint_interval, checkpoint_timesteps, and checkpoint_file validations

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
tests/test_simulation_execution_options.py	Refactored tests for various checkpoint configuration scenarios
kwave/options/simulation_execution_options.py	Moved checkpoint validation into a dedicated method and updated errors
kwave/executor.py	Added a call to validate() before simulation execution

Comments suppressed due to low confidence (1)

kwave/options/simulation_execution_options.py:211

The error message for checkpoint_interval indicates that the value must be a 'positive integer in seconds' even though 0 is treated as valid. Consider updating the message to 'non-negative integer' to better reflect the intended behavior.

if not isinstance(value, int) or value < 0:

coderabbitai · 2025-09-11T17:35:27Z

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds a validation call in Executor.run_simulation to enforce execution options before command build. Introduces centralized checkpoint option validation via validate() and _validate_checkpoint_options() in SimulationExecutionOptions. Updates error conditions/messages. Tests reorganized and expanded to cover new validation rules and edge cases.

Changes

Cohort / File(s)	Summary
Execution flow validation `kwave/executor.py`	Calls `self.execution_options.validate()` in `run_simulation` before building the command; minor comment whitespace change in `parse_executable_output`.
Checkpoint options validation `kwave/options/simulation_execution_options.py`	Adds `validate()` and `_validate_checkpoint_options()`; enforces mutual exclusivity of `checkpoint_interval` and `checkpoint_timesteps`; requires `checkpoint_file` when either is set; requires at least one of the two when a file is provided; tightens positivity rule and messages for `checkpoint_interval`.
Tests for validation rules `tests/test_simulation_execution_options.py`	Refactors and expands tests: granular validation for `checkpoint_interval`, `checkpoint_timesteps`, and `checkpoint_file`; asserts required-file rule; rejects dual-option usage; removes prior scenario allowing both options together.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as Caller
  participant E as Executor
  participant O as SimulationExecutionOptions
  participant P as External Simulator

  U->>E: run_simulation(input)
  E->>O: validate()
  rect rgba(200,230,255,0.3)
    note over O: Centralized checkpoint validation
    O->>O: _validate_checkpoint_options()
  end
  O-->>E: OK
  alt Validation fails
    O-->>E: raise ValueError / FileNotFoundError
    E-->>U: Propagate error
  else Validation passes
    E->>E: build command
    E->>P: launch process
    P-->>E: output
    E-->>U: results
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "Improve checkpointing parameter validation" succinctly and accurately captures the primary change: stricter validation and refactoring of checkpoint-related options in SimulationExecutionOptions. It aligns with the PR objectives and the provided summaries showing addition of validate()/_validate_checkpoint_options, updated error messages, and expanded tests. The phrasing is concise, specific, and free of noise, so a teammate scanning history will understand the main intent.

Poem

I thump my paws: “Validate first!”
No mixed ticks or seconds reversed.
A file to hold each checkpoint crumb,
Or else the run will not become.
With tidy tests and stricter gate,
We sim and snack—commit tastes great! 🥕

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch enhancement/update-logging-valildation

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

kwave/executor.py (1)

41-56: Prevent possible deadlock when streaming stdout.

Reading only stdout line-by-line while stderr is buffered can deadlock if the child writes heavily to stderr. Merge stderr into stdout when streaming and avoid the extra wait(); fall back to communicate() otherwise.

Apply:

-        with subprocess.Popen(
-            command, env=self.execution_options.env_vars, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True
-        ) as proc:
+        with subprocess.Popen(
+            command,
+            env=self.execution_options.env_vars,
+            stdout=subprocess.PIPE,
+            stderr=(subprocess.STDOUT if self.execution_options.show_sim_log else subprocess.PIPE),
+            text=True,
+        ) as proc:
             stdout, stderr = "", ""
-            if self.execution_options.show_sim_log:
-                # Stream stdout in real-time
-                for line in proc.stdout:
-                    print(line, end="")
-
-            stdout, stderr = proc.communicate()
-
-            proc.wait()  # wait for process to finish before checking return code
+            if self.execution_options.show_sim_log:
+                # Stream combined stdout/stderr in real-time
+                lines = []
+                for line in proc.stdout:
+                    print(line, end="")
+                    lines.append(line)
+                stdout = "".join(lines)
+            else:
+                stdout, stderr = proc.communicate()
             if proc.returncode != 0:
                 raise subprocess.CalledProcessError(proc.returncode, command, stdout, stderr)

🧹 Nitpick comments (3)

kwave/options/simulation_execution_options.py (3)

221-225: Fix message: 0 is allowed for timesteps but error says “positive”.

The setter accepts 0 (< 0 check) but the message says “positive”. Make it “non-negative” to avoid confusion.

-        if value is not None:
-            if not isinstance(value, int) or value < 0:
-                raise ValueError("Checkpoint timesteps must be a positive integer")
+        if value is not None:
+            if not isinstance(value, int) or value < 0:
+                raise ValueError("Checkpoint timesteps must be a non-negative integer number of timesteps")

103-105: Minor: missing f-string in error message.

The {value} placeholder won’t interpolate.

-        if not isinstance(value, int):
-            raise ValueError("Got {value}. Number of threads must be 'all' or a positive integer")
+        if not isinstance(value, int):
+            raise ValueError(f"Got {value}. Number of threads must be 'all' or a positive integer")

159-162: Type consistency for binary_path (return Path, accept Path).

Getter is annotated to return Path but returns str when _binary_path is set; setter only accepts str. Normalize to Path to avoid surprises.

     def binary_path(self) -> Path:
-        if self._binary_path is not None:
-            return self._binary_path
+        if self._binary_path is not None:
+            return Path(self._binary_path)

@@
-    def binary_path(self, value: str):
+    def binary_path(self, value: Union[str, Path]):
         # check if the binary path is a valid path
-        if not os.path.exists(value):
+        path = Path(value)
+        if not path.exists():
             raise FileNotFoundError(
-                f"Binary path {value} does not exist. If you are trying to set `binary_dir`, use the `binary_dir` attribute instead."
+                f"Binary path {path} does not exist. If you are trying to set `binary_dir`, use the `binary_dir` attribute instead."
             )
-        self._binary_path = value
+        self._binary_path = path

Also applies to: 174-181

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1311b3 and b3b927c.

📒 Files selected for processing (3)

kwave/executor.py (1 hunks)
kwave/options/simulation_execution_options.py (2 hunks)
tests/test_simulation_execution_options.py (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

kwave/executor.py (1)

kwave/options/simulation_execution_options.py (1)

validate (69-81)

tests/test_simulation_execution_options.py (1)

kwave/options/simulation_execution_options.py (8)

SimulationExecutionOptions (13-333)

checkpoint_file (228-231)

checkpoint_file (234-244)

checkpoint_timesteps (217-218)

checkpoint_timesteps (221-225)

checkpoint_interval (206-207)

checkpoint_interval (210-214)

validate (69-81)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (260)

GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test_install (windows-latest, 3.12, test)
GitHub Check: test_install (windows-latest, 3.13, docs)
GitHub Check: test_install (windows-latest, 3.10, all)
GitHub Check: test_example (windows-latest, 3.13)
GitHub Check: test_example (windows-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.10)
GitHub Check: test_example (windows-latest, 3.11)
GitHub Check: test_example (ubuntu-latest, 3.11)
GitHub Check: test_example (windows-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test_install (windows-latest, 3.12, test)
GitHub Check: test_install (windows-latest, 3.13, docs)
GitHub Check: test_install (windows-latest, 3.10, all)
GitHub Check: test_example (windows-latest, 3.13)
GitHub Check: test_example (windows-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.10)
GitHub Check: test_example (windows-latest, 3.11)
GitHub Check: test_example (ubuntu-latest, 3.11)
GitHub Check: test_example (windows-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test_install (windows-latest, 3.12, test)
GitHub Check: test_install (windows-latest, 3.13, docs)
GitHub Check: test_install (windows-latest, 3.10, all)
GitHub Check: test_example (windows-latest, 3.13)
GitHub Check: test_example (windows-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.10)
GitHub Check: test_example (windows-latest, 3.11)
GitHub Check: test_example (ubuntu-latest, 3.11)
GitHub Check: test_example (windows-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test_install (windows-latest, 3.12, test)
GitHub Check: test_install (windows-latest, 3.13, docs)
GitHub Check: test_install (windows-latest, 3.10, all)
GitHub Check: test_example (windows-latest, 3.13)
GitHub Check: test_example (windows-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.10)
GitHub Check: test_example (windows-latest, 3.11)
GitHub Check: test_example (ubuntu-latest, 3.11)
GitHub Check: test_example (windows-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test_install (windows-latest, 3.12, test)
GitHub Check: test_install (windows-latest, 3.13, docs)
GitHub Check: test_install (windows-latest, 3.10, all)
GitHub Check: test_example (windows-latest, 3.13)
GitHub Check: test_example (windows-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.10)
GitHub Check: test_example (windows-latest, 3.11)
GitHub Check: test_example (ubuntu-latest, 3.11)
GitHub Check: test_example (windows-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test_install (windows-latest, 3.12, test)
GitHub Check: test_install (windows-latest, 3.13, docs)
GitHub Check: test_install (windows-latest, 3.10, all)
GitHub Check: test_example (windows-latest, 3.13)
GitHub Check: test_example (windows-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.10)
GitHub Check: test_example (windows-latest, 3.11)
GitHub Check: test_example (ubuntu-latest, 3.11)
GitHub Check: test_example (windows-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test_install (windows-latest, 3.12, test)
GitHub Check: test_install (windows-latest, 3.13, docs)
GitHub Check: test_install (windows-latest, 3.10, all)
GitHub Check: test_example (windows-latest, 3.13)
GitHub Check: test_example (windows-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.10)
GitHub Check: test_example (windows-latest, 3.11)
GitHub Check: test_example (ubuntu-latest, 3.11)
GitHub Check: test_example (windows-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test_install (windows-latest, 3.12, test)
GitHub Check: test_install (windows-latest, 3.13, docs)
GitHub Check: test_install (windows-latest, 3.10, all)
GitHub Check: test_example (windows-latest, 3.13)
GitHub Check: test_example (windows-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.10)
GitHub Check: test_example (windows-latest, 3.11)
GitHub Check: test_example (ubuntu-latest, 3.11)
GitHub Check: test_example (windows-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test_install (windows-latest, 3.12, test)
GitHub Check: test_install (windows-latest, 3.13, docs)
GitHub Check: test_install (windows-latest, 3.10, all)
GitHub Check: test_example (windows-latest, 3.13)
GitHub Check: test_example (windows-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.10)
GitHub Check: test_example (windows-latest, 3.11)
GitHub Check: test_example (ubuntu-latest, 3.11)
GitHub Check: test_example (windows-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.12)
GitHub Check: test (ubuntu-latest, 3.13)
GitHub Check: test (ubuntu-latest, 3.11)
GitHub Check: test (windows-latest, 3.13)
GitHub Check: test (windows-latest, 3.10)
GitHub Check: test (ubuntu-latest, 3.10)
GitHub Check: test (windows-latest, 3.12)
GitHub Check: test (windows-latest, 3.11)
GitHub Check: test_install (windows-latest, 3.12, test)
GitHub Check: test_install (windows-latest, 3.13, docs)
GitHub Check: test_install (windows-latest, 3.10, all)
GitHub Check: test_example (windows-latest, 3.13)
GitHub Check: test_example (windows-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.12)
GitHub Check: test_example (ubuntu-latest, 3.10)
GitHub Check: test_example (windows-latest, 3.11)
GitHub Check: test_example (ubuntu-latest, 3.11)
GitHub Check: test_example (windows-latest, 3.10)

🔇 Additional comments (4)

kwave/executor.py (1)

36-38: Good guardrail: pre-run validation inserted.

Calling self.execution_options.validate() before building the command is the right place to fail fast on invalid options.

kwave/options/simulation_execution_options.py (2)

54-55: Early validation is appropriate.

Triggering self.validate() at the end of __init__ is safe here (no checkpoint options set by default) and helps fail fast for invalid initializations.

56-68: Centralized checkpoint validation looks correct.

Rules are enforced consistently: file required when either parameter is set, parameters are mutually exclusive, and file alone is disallowed.

Also applies to: 69-81

tests/test_simulation_execution_options.py (1)

272-283: Tests cover the new validation matrix well.

Nice coverage for type/value checks, mutual exclusion, and dependency on checkpoint_file. The assertions on precise error messages for path/extension are spot on.

Also applies to: 299-316, 317-334, 335-362, 363-379

waltsims · 2025-09-11T18:18:51Z

@coderabit-ai review

Improve checkpointing parameter validation

528c1bf

waltsims requested a review from Copilot May 24, 2025 16:29

Copilot AI reviewed May 24, 2025

View reviewed changes

kwave/options/simulation_execution_options.py Show resolved Hide resolved

kwave/options/simulation_execution_options.py Show resolved Hide resolved

validate before simulating

5d0829a

waltsims requested a review from Copilot May 24, 2025 17:17

Simplify and extend tests

1d47860

Copilot AI reviewed May 24, 2025

View reviewed changes

kwave/executor.py Outdated Show resolved Hide resolved

waltsims and others added 5 commits May 24, 2025 10:20

minor fix

17958d9

Co-authored-by: Copilot <[email protected]>

Correct for file not found

747fe3b

Update error message on windows.

125e300

Fix validation

f34b2c9

Revert changes

fe2e7a9

waltsims requested a review from Copilot May 25, 2025 04:31

Copilot AI reviewed May 25, 2025

View reviewed changes

waltsims requested review from Copilot and faridyagubbayli May 25, 2025 04:55

Copilot AI reviewed May 25, 2025

View reviewed changes

Switch to positive values and update tests

005d8b2

waltsims linked an issue May 25, 2025 that may be closed by this pull request

[ENHANCE] Provide checkpoint executable options #592

Open

Merge branch 'master' into enhancement/update-logging-valildation

b3b927c

coderabbitai bot reviewed Sep 11, 2025

View reviewed changes

Merge branch 'master' into enhancement/update-logging-valildation

0cde11d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve checkpointing parameter validation #611

Improve checkpointing parameter validation #611

Uh oh!

waltsims commented May 24, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 24, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

coderabbitai bot commented Sep 11, 2025 •

edited

Loading

Other AI code review bot(s) detected

Uh oh!

coderabbitai bot left a comment

Uh oh!

waltsims commented Sep 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Improve checkpointing parameter validation #611

Are you sure you want to change the base?

Improve checkpointing parameter validation #611

Uh oh!

Conversation

waltsims commented May 24, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refactoring and Validation Enhancements:

Test Suite Improvements:

Summary by CodeRabbit

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

coderabbitai bot commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other AI code review bot(s) detected

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Pre-merge checks (2 passed, 1 warning)

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

waltsims commented Sep 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

waltsims commented May 24, 2025 •

edited by coderabbitai bot

Loading

codecov bot commented May 24, 2025 •

edited

Loading

coderabbitai bot commented Sep 11, 2025 •

edited

Loading