Skip to content

sources.py: matrix_rank check cannot detect non-coplanar Plane bounds #99

Description

@lispandfound

Location: source_modelling/sources.py:272

What happens: Plane's validation claims to reject bounds that do not form a plane:

269: top_mask = np.isclose(self.bounds[:, 2], self.bounds[:, 2].min())
270: top = self.bounds[top_mask]
271: bottom = self.bounds[~top_mask]
272: if (
273:     np.linalg.matrix_rank(self.bounds) != 3
274:     or len(top) != 2
...
277: ):
278:     raise ValueError("Bounds do not form a plane.")

matrix_rank(self.bounds) != 3 cannot detect non-coplanarity. bounds is a 4x3 array of absolute NZTM positions, so its rank is at most 3 and is 3 for essentially any four points not passing through the origin — coplanar or not. Coplanarity is a property of the differences between the points, not of the points as vectors from the origin.

Why that's wrong: The docstring one line above states the requirement — sources.py:260-262: "3. Four points total, spanning exactly a plane. Not a line or a point." — and :278 raises "Bounds do not form a plane." Four corners that clearly do not lie in a common plane are accepted without complaint, and every downstream consumer (the lstsq inversion at sources.py:804, the Plane geometry properties) then silently treats a warped quadrilateral as planar.

How to reproduce:

import numpy as np
from qcore import coordinates
from source_modelling import sources

o = coordinates.wgs_depth_to_nztm(np.array([-43.5, 172.6, 0.0]))
along, down = np.array([10000., 0., 0.]), np.array([0., 5000., 8000.])
bad = np.array([o, o + along, o + along + down, o + down])
bad[2] += np.array([0.0, 4000.0, 0.0])       # lift one corner off the plane

d = bad - bad[0]
print(np.linalg.matrix_rank(d, tol=1e-6))     # 3 -> genuinely NOT coplanar
print(np.linalg.matrix_rank(bad))             # 3 -> the check sees nothing wrong
sources.Plane(bad)                            # accepted

For comparison, a genuinely planar set gives matrix_rank(bounds) == 3 too, so the check has the same value for both cases and carries no information.

Suggested direction: No PR attached, because tightening this is a behaviour change that needs a maintainer's judgement: the correct test is matrix_rank(bounds - bounds[0]) == 2 under an explicit tolerance, but real fault-model corners are frequently a little non-coplanar from rounding and digitisation, so a strict version may start rejecting inputs that currently work. The tolerance is the decision to make — and whether the outcome should be a hard ValueError or a warning plus a projection onto the best-fit plane.

Confidence: high

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    ai-sweepFiled by an automated /bug-sweep runbugSomething isn't workingimpact:lowscope:source_modellingSweep scope: source_modelling package

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions