Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements fixes for QGIS functionality by adding automatic rebuild capabilities to fault geometry, improving numerical stability in interpolation, and enhancing input handling for strike/dip conversions.
Changes:
- Added property setters to FaultBuilder that automatically trigger geometry rebuilds when fault parameters change
- Implemented column scaling matrix and ridge regularization in the discrete interpolator to improve numerical stability
- Enhanced strikedip2vector to properly handle both scalar and array inputs
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| examples/3_fault/plot_update_fault_geometry.py | New example demonstrating dynamic fault geometry updates |
| LoopStructural/utils/maths.py | Added input type conversion for strike/dip parameters |
| LoopStructural/modelling/features/builders/_fault_builder.py | Implemented property-based auto-rebuild system with suspend mechanism |
| LoopStructural/interpolators/_discrete_interpolator.py | Added scaling matrix and ridge regularization for improved numerical stability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(v.shape) != 1 or v.shape[0] != 3: | ||
| raise ValueError("fault_normal_vector must be a 3 element array") | ||
| if v.ndim != 1: | ||
| v = v[0,:] | ||
| if v.shape[0] != 3: | ||
| raise ValueError("fault_normal_vector must be a 3 element array") | ||
| arr = np.array(v, dtype=float) |
There was a problem hiding this comment.
The validation logic is redundant. Lines 144-145 check if v is a 1D array with 3 elements and raise an error if not. However, lines 146-149 then check if v is not 1D and attempt to extract the first row, followed by another check for 3 elements. This creates contradictory logic: if the first check passes (v is 1D with 3 elements), the second check is unnecessary. If the first check fails, an exception is already raised. Consider simplifying to first check v.ndim, extract first row if needed, then validate shape[0] == 3.
| if len(v.shape) != 1 or v.shape[0] != 3: | |
| raise ValueError("fault_normal_vector must be a 3 element array") | |
| if v.ndim != 1: | |
| v = v[0,:] | |
| if v.shape[0] != 3: | |
| raise ValueError("fault_normal_vector must be a 3 element array") | |
| arr = np.array(v, dtype=float) | |
| arr = np.array(v, dtype=float) | |
| # If more than 1D, take the first row and then validate length | |
| if arr.ndim > 1: | |
| arr = arr[0] | |
| if arr.shape[0] != 3: | |
| raise ValueError("fault_normal_vector must be a 3 element array") |
| # self._post_solve() | ||
| # apply scaling matrix to solution | ||
| if self.apply_scaling_matrix: | ||
| self.c = S @ self.c |
There was a problem hiding this comment.
Variable S is only defined when self.apply_scaling_matrix is True (line 621), but this code path can be reached through multiple solver branches (callable, cg, lsmr, admm, unknown solver) where S might not be in scope. If apply_scaling_matrix is False initially but somehow becomes True, or if an error path is taken, S will be undefined. The variable should be initialized to None at the start of the method, or this block should be moved inside the earlier if self.apply_scaling_matrix block.
| self.c = S @ self.c | |
| S = locals().get("S", None) | |
| if S is None: | |
| logger.error("Scaling matrix S is not defined; skipping scaling of solution.") | |
| self.up_to_date = False | |
| else: | |
| self.c = S @ self.c |
| self.type = InterpolatorType.BASE_DISCRETE | ||
|
|
||
| self.apply_scaling_matrix = True | ||
| self.add_ridge_regulatisation = True |
There was a problem hiding this comment.
Corrected spelling of 'regulatisation' to 'regularisation'.
| self.add_ridge_regulatisation = True | |
| self.add_ridge_regularisation = True |
| ridge = sparse.eye(A.shape[1]) * self.ridge_factor | ||
| A = sparse.vstack([A, ridge]) | ||
| b = np.hstack([b, np.zeros(A.shape[1])]) | ||
| logger.info("Adding ridge regularisation to interpolation matrix") |
There was a problem hiding this comment.
Corrected spelling of 'regularisation' to 'regularization' for consistency with American English spelling used elsewhere in scientific computing libraries.
| logger.info("Adding ridge regularisation to interpolation matrix") | |
| logger.info("Adding ridge regularization to interpolation matrix") |
| # if 'atol' not in solver_kwargs: | ||
| # if tol is not None: | ||
| # solver_kwargs['atol'] = tol |
There was a problem hiding this comment.
This comment appears to contain commented-out code.
| # if 'atol' not in solver_kwargs: | |
| # if tol is not None: | |
| # solver_kwargs['atol'] = tol |
| if 'btol' not in solver_kwargs: | ||
| if tol is not None: | ||
| solver_kwargs['btol'] = tol | ||
| solver_kwargs['atol'] = 0. |
There was a problem hiding this comment.
This expression mutates a default value.
This expression mutates a default value.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@lachlangrose I've opened a new pull request, #278, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
No description provided.