Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip You can customize the tone of the review comments and chat replies.Set the ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Docstrings generation - SUCCESS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/pyVertexModel/algorithm/vertexModel.py`:
- Around line 255-265: The code currently always builds filename with
os.path.join(PROJECT_DIRECTORY, self.set.initial_filename_state) which turns
None into the string "None"; update initialize to first check if img_input is
None and self.set.initial_filename_state is None and handle that case (either
skip file-based initialization or raise a clear ValueError), and only call
os.path.join when self.set.initial_filename_state is not None; ensure any error
log references the actual missing filename or explicitly states that
initial_filename_state is unset; adjust the FileNotFoundError path check
accordingly in initialize.
In `@src/pyVertexModel/geometry/geo.py`:
- Around line 369-370: The membership check "if '/Temp/' not in
c_set.initial_filename_state" can raise TypeError when
c_set.initial_filename_state is None; update the condition used before calling
update_lmin0 (and the default_value call to find_lmin0_equal_target_gr) to guard
against None by first checking truthiness of c_set.initial_filename_state (e.g.,
treat None the same as "does not contain '/Temp/'"); modify the conditional
around update_lmin0 accordingly so it safely handles
c_set.initial_filename_state == None and still calls
update_lmin0(default_value=find_lmin0_equal_target_gr(self, c_set)) when
appropriate.
🧹 Nitpick comments (1)
src/pyVertexModel/algorithm/vertexModel.py (1)
1294-1302: Fragile check for temporary folder path.The check
'Temp' in self.set.OutputFoldercould match unintended paths (e.g.,/home/user/Temperature/outputor/data/TempResults/). Consider using a more specific check that matches the expected path structure.♻️ Proposed fix for more robust path checking
def clean_temporary_folder(self): """ Clean temporary folder after the simulation. :return: """ - if self.set.OutputFolder is not None and 'Temp' in self.set.OutputFolder: + temp_base = os.path.join(PROJECT_DIRECTORY, 'Temp') + if self.set.OutputFolder is not None and self.set.OutputFolder.startswith(temp_base): shutil.rmtree(self.set.OutputFolder) logger.info(f'Removed temporary output folder at: {self.set.OutputFolder}') self.set.OutputFolder = None
| def initialize(self, img_input=None): | ||
| """ | ||
| Initialize the geometry and the topology of the model. | ||
| :param img_input: Optional. Either a filename (str) or a numpy array containing the image. | ||
| If None, uses the filename from settings. | ||
| """ | ||
| filename = os.path.join(PROJECT_DIRECTORY, self.set.initial_filename_state) | ||
|
|
||
| if not os.path.exists(filename): | ||
| if not os.path.exists(filename) and img_input is None: | ||
| logging.error(f'File {filename} not found') | ||
| raise FileNotFoundError(f'File {filename} not found') |
There was a problem hiding this comment.
Potential issue when initial_filename_state is None.
When initial_filename_state is None (the new default), os.path.join(PROJECT_DIRECTORY, None) will convert None to the string 'None', resulting in a path like /path/to/project/None. This creates a confusing error message since the path won't exist.
Consider adding an early check for None:
🐛 Proposed fix to handle None gracefully
def initialize(self, img_input=None):
"""
Initialize the geometry and the topology of the model.
:param img_input: Optional. Either a filename (str) or a numpy array containing the image.
If None, uses the filename from settings.
"""
+ if self.set.initial_filename_state is None and img_input is None:
+ raise ValueError("Either initial_filename_state must be set or img_input must be provided")
+
filename = os.path.join(PROJECT_DIRECTORY, self.set.initial_filename_state)
if not os.path.exists(filename) and img_input is None:
logging.error(f'File {filename} not found')
raise FileNotFoundError(f'File {filename} not found')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def initialize(self, img_input=None): | |
| """ | |
| Initialize the geometry and the topology of the model. | |
| :param img_input: Optional. Either a filename (str) or a numpy array containing the image. | |
| If None, uses the filename from settings. | |
| """ | |
| filename = os.path.join(PROJECT_DIRECTORY, self.set.initial_filename_state) | |
| if not os.path.exists(filename): | |
| if not os.path.exists(filename) and img_input is None: | |
| logging.error(f'File {filename} not found') | |
| raise FileNotFoundError(f'File {filename} not found') | |
| def initialize(self, img_input=None): | |
| """ | |
| Initialize the geometry and the topology of the model. | |
| :param img_input: Optional. Either a filename (str) or a numpy array containing the image. | |
| If None, uses the filename from settings. | |
| """ | |
| if self.set.initial_filename_state is None and img_input is None: | |
| raise ValueError("Either initial_filename_state must be set or img_input must be provided") | |
| filename = os.path.join(PROJECT_DIRECTORY, self.set.initial_filename_state) | |
| if not os.path.exists(filename) and img_input is None: | |
| logging.error(f'File {filename} not found') | |
| raise FileNotFoundError(f'File {filename} not found') |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 265-265: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@src/pyVertexModel/algorithm/vertexModel.py` around lines 255 - 265, The code
currently always builds filename with os.path.join(PROJECT_DIRECTORY,
self.set.initial_filename_state) which turns None into the string "None"; update
initialize to first check if img_input is None and
self.set.initial_filename_state is None and handle that case (either skip
file-based initialization or raise a clear ValueError), and only call
os.path.join when self.set.initial_filename_state is not None; ensure any error
log references the actual missing filename or explicitly states that
initial_filename_state is unset; adjust the FileNotFoundError path check
accordingly in initialize.
| if '/Temp/' not in c_set.initial_filename_state: | ||
| self.update_lmin0(default_value=find_lmin0_equal_target_gr(self, c_set)) |
There was a problem hiding this comment.
TypeError when initial_filename_state is None.
Since initial_filename_state now defaults to None (as changed in set.py), this condition will raise a TypeError when checking '/Temp/' not in None. The in operator doesn't work with None.
🐛 Proposed fix to handle None case
- if '/Temp/' not in c_set.initial_filename_state:
+ if c_set.initial_filename_state is not None and '/Temp/' not in c_set.initial_filename_state:
self.update_lmin0(default_value=find_lmin0_equal_target_gr(self, c_set))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if '/Temp/' not in c_set.initial_filename_state: | |
| self.update_lmin0(default_value=find_lmin0_equal_target_gr(self, c_set)) | |
| if c_set.initial_filename_state is not None and '/Temp/' not in c_set.initial_filename_state: | |
| self.update_lmin0(default_value=find_lmin0_equal_target_gr(self, c_set)) |
🤖 Prompt for AI Agents
In `@src/pyVertexModel/geometry/geo.py` around lines 369 - 370, The membership
check "if '/Temp/' not in c_set.initial_filename_state" can raise TypeError when
c_set.initial_filename_state is None; update the condition used before calling
update_lmin0 (and the default_value call to find_lmin0_equal_target_gr) to guard
against None by first checking truthiness of c_set.initial_filename_state (e.g.,
treat None the same as "does not contain '/Temp/'"); modify the conditional
around update_lmin0 accordingly so it safely handles
c_set.initial_filename_state == None and still calls
update_lmin0(default_value=find_lmin0_equal_target_gr(self, c_set)) when
appropriate.
…ving-nothing-or-all-temporary` Docstrings generation was requested by @Pablo1990. * #10 (comment) The following files were modified: * `src/pyVertexModel/algorithm/vertexModel.py` * `src/pyVertexModel/geometry/geo.py` * `src/pyVertexModel/parameters/set.py`
There was a problem hiding this comment.
Pull request overview
This pull request attempts to enable the system to work without an output folder, allowing users to run simulations without saving intermediate files or by using temporary folders. The changes make both OutputFolder and initial_filename_state optional by setting them to None by default.
Changes:
- Set
initial_filename_statetoNoneby default instead of 'Input/wing_disc_150.mat' - Added None checks for
OutputFolderin redirect_output, iterate_over_time, and save_v_model_state methods - Added conditional logic to skip expensive computation when loading from temporary files
- Added
create_temporary_folder()andclean_temporary_folder()methods for managing temporary folders - Added
Temp/directory to .gitignore
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/pyVertexModel/parameters/set.py | Changed initial_filename_state default to None and added None guard to redirect_output method |
| src/pyVertexModel/geometry/geo.py | Added condition to skip computation for temporary files (contains bug) |
| src/pyVertexModel/algorithm/vertexModel.py | Added None checks for OutputFolder, FileNotFoundError raising, temporary folder management methods, and fixed whitespace |
| .gitignore | Added Temp/ directory to ignore list |
Comments suppressed due to low confidence (1)
src/pyVertexModel/algorithm/vertexModel.py:271
- When initial_filename_state is None (the new default), os.path.join will convert None to the string 'None', creating an invalid path like 'PROJECT_DIRECTORY/None'. This causes several problems: (1) Line 267's os.path.splitext will operate on this invalid path, (2) Lines 269-271 will create invalid output_filename paths, and (3) Lines 273-295 will use these invalid paths for file operations. Even with the added check at lines 263-265, if img_input is provided but initial_filename_state is None, the code will continue with the invalid filename. The method should first check if initial_filename_state is None and handle it appropriately before attempting any path operations.
filename = os.path.join(PROJECT_DIRECTORY, self.set.initial_filename_state)
if not os.path.exists(filename) and img_input is None:
logging.error(f'File {filename} not found')
raise FileNotFoundError(f'File {filename} not found')
base, ext = os.path.splitext(filename)
if self.set.min_3d_neighbours is None:
output_filename = f"{base}_{self.set.TotalCells}cells_{self.set.CellHeight}_scutoids_{self.set.percentage_scutoids}.pkl"
else:
output_filename = f"{base}_{self.set.TotalCells}cells_{self.set.CellHeight}_min3d_{self.set.min_3d_neighbours}.pkl"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.set.OutputFolder is not None and 'Temp' in self.set.OutputFolder: | ||
| shutil.rmtree(self.set.OutputFolder) | ||
| logger.info(f'Removed temporary output folder at: {self.set.OutputFolder}') | ||
| self.set.OutputFolder = None No newline at end of file |
There was a problem hiding this comment.
The logic for checking temporary folders using string matching with 'Temp' is fragile and could lead to unintended behavior. If a user creates an output folder with 'Temp' anywhere in its path (e.g., 'MyTempResults' or '/home/user/Temporary/output'), this method would incorrectly delete it. Consider using a more robust approach, such as storing a flag indicating whether the folder is temporary, or checking if the folder is specifically within the PROJECT_DIRECTORY/Temp directory.
| if self.set.OutputFolder is not None and 'Temp' in self.set.OutputFolder: | |
| shutil.rmtree(self.set.OutputFolder) | |
| logger.info(f'Removed temporary output folder at: {self.set.OutputFolder}') | |
| self.set.OutputFolder = None | |
| if self.set.OutputFolder is None: | |
| return | |
| output_folder = os.path.abspath(self.set.OutputFolder) | |
| temp_root = os.path.abspath(os.path.join(PROJECT_DIRECTORY, 'Temp')) | |
| try: | |
| is_within_temp = os.path.commonpath([output_folder, temp_root]) == temp_root | |
| except ValueError: | |
| # If paths are on different drives or otherwise incomparable, do not delete. | |
| is_within_temp = False | |
| if is_within_temp and output_folder != temp_root and os.path.isdir(output_folder): | |
| shutil.rmtree(output_folder) | |
| logger.info(f'Removed temporary output folder at: {self.set.OutputFolder}') | |
| else: | |
| logger.warning(f"Output folder '{self.set.OutputFolder}' not removed because it is not a managed temporary folder.") | |
| self.set.OutputFolder = None |
| # Update lmin0 with the minimum value in lmin_values | ||
| self.update_lmin0() | ||
| self.update_lmin0(default_value=find_lmin0_equal_target_gr(self, c_set)) | ||
| if '/Temp/' not in c_set.initial_filename_state: |
There was a problem hiding this comment.
This condition attempts to check if the initial_filename_state contains '/Temp/', but it will raise a TypeError when initial_filename_state is None because the 'in' operator cannot be used with None. The logic should first check if initial_filename_state is not None before checking if it contains '/Temp/'. A safer approach would be: 'if c_set.initial_filename_state is not None and '/Temp/' not in c_set.initial_filename_state:'
| if '/Temp/' not in c_set.initial_filename_state: | |
| if c_set.initial_filename_state is not None and '/Temp/' not in c_set.initial_filename_state: |
|
|
||
| def create_temporary_folder(self): | ||
| """ | ||
| Create temporary folder where store every require file during the simulation. |
There was a problem hiding this comment.
Spelling error: 'require' should be 'required'. The docstring should read 'Create temporary folder where every required file is stored during the simulation.'
| Create temporary folder where store every require file during the simulation. | |
| Create temporary folder where every required file is stored during the simulation. |
Summary by CodeRabbit
New Features
Bug Fixes
Chores