5 fix issue with geometry breaking from time to time #6
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughTests and test-loading were refactored: several vertex-model test functions removed and three behavioral tests added; test data paths and loading now support both .mat and .pkl; output file I/O is guarded by OutputFolder checks; a configurable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@copilot this is a complex task, but these three tests are not passing and I don't know why. The model breaks in those situations where a cell has been remodelled and now it is an scutoid. I want them to be scutoids, but there is something wrong in the vertices (cell.Y) or the forces that makes the model go wild as seen in the tests.
|
|
@Pablo1990 I've opened a new pull request, #7, to work on those changes. Once the pull request is ready, I'll request review from you. |
![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 536-538: The code dereferences self.set.dt_tolerance directly
which breaks for legacy pickled Set objects and allows non‑positive values to
cause endless halving; fix by reading a validated tolerance first (e.g. tol =
getattr(self.set, "dt_tolerance", DEFAULT_DT_TOLERANCE)), clamp/replace any
non‑positive or otherwise invalid values (e.g. if tol is None or tol <= 0: tol =
DEFAULT_DT_TOLERANCE), then use that validated tol in the conditional that
references dt_tolerance (the if block that checks (self.set.dt / self.set.dt0) >
... and resets self.set.MaxIter). Define a clear DEFAULT_DT_TOLERANCE constant
or use an existing config value and update the condition to use tol instead of
self.set.dt_tolerance.
In `@Tests/tests.py`:
- Around line 26-31: The current check on set_test.OutputFolder only compares to
b'' and None, but scipy.io.loadmat may return '' (str) or b'' (bytes) so empty
values can slip through; update the logic that sets set_test.OutputFolder (for
the Set(...) result) to treat None, b'' and '' as empty by normalizing bytes to
str (or checking both types) and then assigning '../Result/Test' when the
normalized value is empty; reference Set and set_test.OutputFolder to locate the
code and ensure the check uses isinstance or explicit comparisons for both b''
and '' before assigning the default OutputFolder.
| if (self.set.iter >= self.set.MaxIter and | ||
| (self.set.dt / self.set.dt0) > 1e-6): | ||
| (self.set.dt / self.set.dt0) > self.set.dt_tolerance): | ||
| self.set.MaxIter = self.set.MaxIter0 |
There was a problem hiding this comment.
Guard dt_tolerance for legacy pickles / invalid values.
Line 536 now dereferences self.set.dt_tolerance; pickled Set objects created before this field existed will raise AttributeError, and non‑positive values can lead to endless halving. Consider a safe default + validation.
🛠️ Suggested fix
- if (self.set.iter >= self.set.MaxIter and
- (self.set.dt / self.set.dt0) > self.set.dt_tolerance):
+ dt_tol = getattr(self.set, "dt_tolerance", 1e-6)
+ if dt_tol <= 0:
+ logger.warning("Invalid dt_tolerance=%s; defaulting to 1e-6", dt_tol)
+ dt_tol = 1e-6
+ if (self.set.iter >= self.set.MaxIter and
+ (self.set.dt / self.set.dt0) > dt_tol):🤖 Prompt for AI Agents
In `@src/pyVertexModel/algorithm/vertexModel.py` around lines 536 - 538, The code
dereferences self.set.dt_tolerance directly which breaks for legacy pickled Set
objects and allows non‑positive values to cause endless halving; fix by reading
a validated tolerance first (e.g. tol = getattr(self.set, "dt_tolerance",
DEFAULT_DT_TOLERANCE)), clamp/replace any non‑positive or otherwise invalid
values (e.g. if tol is None or tol <= 0: tol = DEFAULT_DT_TOLERANCE), then use
that validated tol in the conditional that references dt_tolerance (the if block
that checks (self.set.dt / self.set.dt0) > ... and resets self.set.MaxIter).
Define a clear DEFAULT_DT_TOLERANCE constant or use an existing config value and
update the condition to use tol instead of self.set.dt_tolerance.
| if 'Set' in mat_info.keys(): | ||
| set_test = Set(mat_info['Set']) | ||
| if set_test.OutputFolder.__eq__(b'') or set_test.OutputFolder is None: | ||
| set_test.OutputFolder = '../Result/Test' | ||
| else: | ||
| set_test = None |
There was a problem hiding this comment.
Normalize empty OutputFolder values ('' vs b'').
scipy.io.loadmat can return '' (str) as well as b'' (bytes). If '' slips through, later OutputFolder is not None checks can still trigger filesystem writes to CWD. Consider a broader empty check.
🛠️ Suggested fix
- if set_test.OutputFolder.__eq__(b'') or set_test.OutputFolder is None:
- set_test.OutputFolder = '../Result/Test'
+ if (set_test.OutputFolder is None or
+ (isinstance(set_test.OutputFolder, (bytes, str)) and len(set_test.OutputFolder) == 0)):
+ set_test.OutputFolder = '../Result/Test'📝 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 'Set' in mat_info.keys(): | |
| set_test = Set(mat_info['Set']) | |
| if set_test.OutputFolder.__eq__(b'') or set_test.OutputFolder is None: | |
| set_test.OutputFolder = '../Result/Test' | |
| else: | |
| set_test = None | |
| if 'Set' in mat_info.keys(): | |
| set_test = Set(mat_info['Set']) | |
| if (set_test.OutputFolder is None or | |
| (isinstance(set_test.OutputFolder, (bytes, str)) and len(set_test.OutputFolder) == 0)): | |
| set_test.OutputFolder = '../Result/Test' | |
| else: | |
| set_test = None |
🤖 Prompt for AI Agents
In `@Tests/tests.py` around lines 26 - 31, The current check on
set_test.OutputFolder only compares to b'' and None, but scipy.io.loadmat may
return '' (str) or b'' (bytes) so empty values can slip through; update the
logic that sets set_test.OutputFolder (for the Set(...) result) to treat None,
b'' and '' as empty by normalizing bytes to str (or checking both types) and
then assigning '../Result/Test' when the normalized value is empty; reference
Set and set_test.OutputFolder to locate the code and ensure the check uses
isinstance or explicit comparisons for both b'' and '' before assigning the
default OutputFolder.
There was a problem hiding this comment.
Pull request overview
This PR aims to fix intermittent “geometry breaking / vertices going wild” failures by making time-step backoff configurable and by preventing file I/O during tests that load saved model states.
Changes:
- Introduces a configurable
dt_tolerance(used to decide whetherdtcan keep shrinking) and applies it in the non-convergence logic. - Makes
VertexModelskip VTK/image/state outputs whenset.OutputFolderisNone(enabling non-I/O tests). - Adds new regression tests + many new
.mat/.pklfixtures underTests_data/, and updates test data loading paths.
Reviewed changes
Copilot reviewed 4 out of 79 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pyVertexModel/parameters/set.py | Adds dt_tolerance default and updates cyst fixture path. |
| src/pyVertexModel/algorithm/vertexModel.py | Gates output by OutputFolder and uses configurable dt_tolerance in time-step reduction. |
| Tests/tests.py | Extends load_data to support .pkl model states and updates test data paths. |
| Tests/test_vertexModel.py | Updates fixture paths and adds 3 new regression tests for “vertices going wild”. |
| Tests_data/* | Adds new fixtures required by updated and new tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| file_name = 'LblImg_imageSequence.mat' | ||
| test_dir = 'Tests/data/%s' % file_name | ||
| test_dir = 'Tests/Tests_data/%s' % file_name | ||
| if exists(test_dir): | ||
| Twg_test, X_test = vModel_test.obtain_initial_x_and_tetrahedra(test_dir) | ||
| else: | ||
| Twg_test, X_test = vModel_test.obtain_initial_x_and_tetrahedra('data/%s' % file_name) | ||
| Twg_test, X_test = vModel_test.obtain_initial_x_and_tetrahedra('Tests_data/%s' % file_name) |
There was a problem hiding this comment.
These tests reference Tests/Tests_data/..., but fixtures are under the repository root Tests_data/. Because the fallback uses a relative path, this can fail when tests aren't executed from the repo root. Prefer using an absolute path such as os.path.join(TEST_DIRECTORY, 'Tests_data', file_name) for both branches (and remove the exists() check entirely).
| file_name = 'voronoi_40cells.pkl' | ||
| test_dir = TEST_DIRECTORY + '/Tests/data/%s' % file_name | ||
| test_dir = TEST_DIRECTORY + '/Tests/Tests_data/%s' % file_name | ||
| if exists(test_dir): | ||
| vModel_test.set.initial_filename_state = test_dir | ||
| else: | ||
| vModel_test.set.initial_filename_state = 'data/%s' % file_name | ||
| vModel_test.set.initial_filename_state = 'Tests_data/%s' % file_name |
There was a problem hiding this comment.
test_dir is built as TEST_DIRECTORY + '/Tests/Tests_data/...', but the repository data directory is TEST_DIRECTORY/Tests_data. As written, the exists() check will always fail in a normal checkout, and the code relies on a relative 'Tests_data/...' path (CWD-dependent). Use os.path.join(TEST_DIRECTORY, 'Tests_data', file_name) consistently.
| # Run for 10 iterations. dt should not decrease to 1e-1 | ||
| vModel_test.set.tend = vModel_test.t + 20 * vModel_test.set.dt0 | ||
|
|
There was a problem hiding this comment.
The comment says “Run for 10 iterations”, but tend is set to t + 20 * dt0 (20 time steps at dt0). Either adjust the comment or the tend calculation so the test intent is unambiguous.
| if (self.set.iter >= self.set.MaxIter and | ||
| (self.set.dt / self.set.dt0) > 1e-6): | ||
| (self.set.dt / self.set.dt0) > self.set.dt_tolerance): |
There was a problem hiding this comment.
Backward-compatibility risk: self.set.dt_tolerance is a new attribute. If a VertexModel is restored from an older .pkl where the Set instance predates this field, iteration_did_not_converged will raise AttributeError. Consider using getattr(self.set, 'dt_tolerance', 1e-6) here (or ensure Set defines it in a post-load hook) to keep old saved states runnable.
| test_dir = abspath('Tests/Tests_data/%s' % file_name) | ||
| if file_name.endswith('.mat'): | ||
| if exists(test_dir): | ||
| mat_info = scipy.io.loadmat(test_dir) | ||
| else: | ||
| mat_info = scipy.io.loadmat('Tests_data/%s' % file_name) |
There was a problem hiding this comment.
load_data builds test_dir using abspath('Tests/Tests_data/...'), but the repository test fixtures live under the top-level Tests_data/ directory. This makes the helper depend on the current working directory and the fallback relative path, which can break when tests are run from a different CWD. Prefer building an absolute path from Tests.TEST_DIRECTORY (or Path(__file__).resolve()) and always loading from .../Tests_data/<file>.
| v_model.set.export_images = False | ||
| v_model.set.VTK = False | ||
|
|
||
| return v_model |
There was a problem hiding this comment.
load_data returns a (geo, set, mat_info) tuple for .mat files but returns a VertexModel... instance for .pkl files. This polymorphic return type is easy to misuse (callers can accidentally unpack a .pkl result and fail at runtime). Consider splitting into load_mat_data(...) and load_model_state(...), or always returning a consistent tuple (e.g., (None, None, v_model) or (geo, set, extra) with a documented contract).
| return v_model | |
| return None, None, v_model |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Tests/tests.py`:
- Around line 53-54: The raised FileNotFoundError for non-.mat/.pkl inputs is
misleading; change the exception to a more appropriate type (e.g., ValueError or
a custom UnsupportedFileTypeError) and update the message to indicate an
unsupported file extension including the offending file_name or its extension;
locate the raise statement that references file_name in Tests/tests.py and
replace it with a descriptive "Unsupported file type" exception and message.
🧹 Nitpick comments (3)
Tests/__init__.py (1)
5-7: TEST_DIRECTORY and PROJECT_DIRECTORY are computed identically.Both variables use the same expression
os.path.dirname(os.path.dirname(os.path.abspath(__file__))). If they serve different purposes, consider clarifying with comments or distinct logic. If PROJECT_DIRECTORY is meant to allow override via environment variable while TEST_DIRECTORY is the fixed path, the naming and intent could be documented.Tests/tests.py (2)
15-21: Path fallback logic may produce confusing errors for missing .mat files.If
test_dirdoesn't exist and the fallback path'Tests_data/%s'also doesn't exist,scipy.io.loadmatwill raise its ownFileNotFoundErrorwith the fallback path. This could be confusing since the error won't mention the primary path that was tried first.Consider adding an existence check for the fallback path or logging which path is being used:
♻️ Suggested improvement
def load_data(file_name, return_geo=True): test_dir = abspath('Tests/Tests_data/%s' % file_name) if file_name.endswith('.mat'): if exists(test_dir): mat_info = scipy.io.loadmat(test_dir) - else: + elif exists('Tests_data/%s' % file_name): mat_info = scipy.io.loadmat('Tests_data/%s' % file_name) + else: + raise FileNotFoundError('MAT file %s not found in Tests/Tests_data or Tests_data' % file_name)
40-45: Same fallback pattern without existence check for .pkl files.Similar to the .mat handling, if neither path exists for a .pkl file,
load_statewill fail with an error that may not clearly indicate both paths were tried.
| else: | ||
| geo_test = None | ||
| set_test = None | ||
| raise FileNotFoundError('File %s not found' % file_name) |
There was a problem hiding this comment.
Error message is misleading for unsupported file extensions.
FileNotFoundError with "not found" message is raised for files that don't end with .mat or .pkl, but this is actually an "unsupported file type" error, not a "file not found" error. This could mislead debugging efforts.
♻️ Suggested fix
else:
- raise FileNotFoundError('File %s not found' % file_name)
+ raise ValueError('Unsupported file type: %s (expected .mat or .pkl)' % file_name)📝 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.
| else: | |
| geo_test = None | |
| set_test = None | |
| raise FileNotFoundError('File %s not found' % file_name) | |
| else: | |
| raise ValueError('Unsupported file type: %s (expected .mat or .pkl)' % file_name) |
🤖 Prompt for AI Agents
In `@Tests/tests.py` around lines 53 - 54, The raised FileNotFoundError for
non-.mat/.pkl inputs is misleading; change the exception to a more appropriate
type (e.g., ValueError or a custom UnsupportedFileTypeError) and update the
message to indicate an unsupported file extension including the offending
file_name or its extension; locate the raise statement that references file_name
in Tests/tests.py and replace it with a descriptive "Unsupported file type"
exception and message.
I have prepared some tests that should pass, but they are not passing:
Summary by CodeRabbit
New Features
Tests
Chores