Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to generalize how SMS++ solver command-line options are handled by consolidating solver tool initialization/CLI generation in SMSPPSolverTool, and updates tests to align with the new calling patterns.
Changes:
- Refactors
SMSPPSolverToolto use a generalizedfp_solverexecutable path and**kwargsfor solver options, removing per-solvercalculate_executable_call()overrides. - Adjusts solver tool constructors to a common signature (adds
configsolution,fp_solver, etc.). - Updates optimization tests to match the updated option passing (e.g., removing
logging=TruefromSMSNetwork.optimize()calls).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
pysmspp/smspp_tools.py |
Major refactor of solver tool API and command construction to support generalized options. |
test/test_optimize.py |
Updates tests to reflect changed solver invocation/argument ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| None if configfile is None else str(Path(configfile).resolve()) | ||
| ) | ||
| self.configsolution = ( | ||
| None if configsolution is None else str(configsolution.resolve().name) |
There was a problem hiding this comment.
configsolution is typed as Path | str, but the assignment uses configsolution.resolve() directly. Passing a string will raise AttributeError. Consider normalizing with Path(configsolution).resolve() (like the other path params) and store the full resolved path (not only .name) since -C currently receives just the basename.
| None if configsolution is None else str(configsolution.resolve().name) | |
| None if configsolution is None else str(Path(configsolution).resolve()) |
| self._kwargs = kwargs | ||
|
|
||
| if "o" not in self._kwargs: | ||
| self._kwargs["o"] = "" |
There was a problem hiding this comment.
Docstring says -o/-O are added only when fp_solution is provided, but __init__ unconditionally injects o into _kwargs, so calculate_executable_call() always appends -o even when fp_solution is None. This both changes default solver behavior and can lead to duplicate -o when fp_solution is set; consider removing the default or handling -o as a valueless flag separately from kwargs.
| When provided, option "-C" is added to the executable call to specify the configuration solution file. | ||
| fp_log : Path | str, optional | ||
| Path to the log file, by default None. | ||
| When provided, the solver log is saved to the specified solver, by default None. |
There was a problem hiding this comment.
The fp_log parameter description says the log is saved to the "specified solver"; this reads like a typo and is confusing. It should describe saving the log to the specified file path.
| When provided, the solver log is saved to the specified solver, by default None. | |
| When provided, the solver log is saved to the specified file path, by default None. |
| def calculate_executable_call(self): | ||
| """ | ||
| Calculate the executable call to run the solver tool. | ||
|
|
||
| This method is meant to be overridden by subclasses. The base class | ||
| implementation calls the function provided during initialization. | ||
| Generate the command-line call for the UCBlock solver. | ||
|
|
||
| Notes | ||
| ----- | ||
| Subclasses override this method to return solver-specific command strings. | ||
| Returns | ||
| ------- | ||
| str | ||
| The command string to execute the ucblock_solver. | ||
| """ | ||
| raise NotImplementedError( | ||
| "Method calculate_executable_call must be implemented in the derived class." | ||
| ) | ||
| configdir, configfile = os.path.split(Path(self.configfile).resolve()) | ||
| networkdir, networkfile = os.path.split(Path(self.fp_network).resolve()) | ||
| exec_path = f"{self._fp_solver} {networkfile} -p {networkdir}/ -c {configdir}/ -S {configfile}" | ||
| if self.fp_solution is not None: |
There was a problem hiding this comment.
calculate_executable_call() is implemented in the base class but its docstring and behavior are UCBlock-specific (it refers to UCBlock/ucblock_solver and hard-codes -p/-c/-S layout). Since subclasses no longer override this, either make this method truly solver-agnostic (and document the required common CLI contract) or restore per-solver overrides to avoid silently generating invalid command lines for other tools.
| def __init__( | ||
| self, | ||
| fp_network: Path | str = "", | ||
| configfile: Path | str = "", | ||
| fp_solution: Path | str = None, | ||
| fp_network: Path | str = None, | ||
| fp_log: Path | str = None, | ||
| configfile: Path | str = None, | ||
| fp_solution: Path | str = None, |
There was a problem hiding this comment.
Constructor parameter order changed (e.g., configfile moved after fp_log). This is a breaking change for any callers using positional arguments. To preserve compatibility, consider keeping the previous positional order or making the parameters keyword-only (e.g., by adding * after self).
1 participant
No description provided.