Skip to content

Commit 537da39

Browse files
lachlangroseclaude
andauthored
test: add a regression test for stratigraphic values (#113)
* test: pin training-value/isovalue direction agreement for stratigraphic columns Guards against the swap fixed in 814be12: model_manager.py's per-unit training value and LoopStructural's get_isovalues() must agree on which direction values increase, or extracted isosurfaces get labelled with the wrong unit while keeping correct geometry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: add regression test for undigitised unit affecting later units' trained values * style: fix black formatting and ruff lint (unnecessary generator) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: training values no longer disagree with get_isovalues() direction update_foliation_features re-reversed group.units, which get_groups() already returns in the order get_isovalues() walks, so every basal contact was trained with the wrong scalar value. Units with no digitised data also skipped accumulating their thickness via continue, shifting every later unit's trained value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 814be12 commit 537da39

6 files changed

Lines changed: 167 additions & 17 deletions

File tree

loopstructural/gui/map2loop_tools/fault_topology_widget.py

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ def _run_topology(self):
174174
# not just the ones map2loop found a relationship for. A fault with
175175
# no detected topological relationship is still a real fault and
176176
# must not be dropped from the fault topology.
177-
new_faults = set(str(v) for v in gdf['ID'].unique())
177+
new_faults = {str(v) for v in gdf['ID'].unique()}
178178

179179
# Add new faults; never remove existing ones here, so faults
180180
# without a detected relationship (or ones the user added
@@ -209,9 +209,7 @@ def _run_topology(self):
209209
else:
210210
f1 = str(row.iloc[0])
211211
f2 = str(row.iloc[1])
212-
ft.update_fault_relationship(
213-
f1, f2, FaultRelationshipType.ABUTTING
214-
)
212+
ft.update_fault_relationship(f1, f2, FaultRelationshipType.ABUTTING)
215213
except Exception:
216214
pass
217215

loopstructural/gui/modelling/model_definition/bounding_box.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,11 @@ def __init__(self, parent=None, data_manager=None):
3232
self.selectFromCurrentLayerButton, "mActionZoomToLayer.svg", "Select from Current Layer"
3333
)
3434
self._style_tool_button(
35-
self.useCurrentViewExtentButton, "mActionSetToCanvasExtent.svg", "Use Current View Extent"
36-
)
37-
self._style_tool_button(
38-
self.drawOnMapButton, "mActionAddBasicRectangle.svg", "Draw on Map"
35+
self.useCurrentViewExtentButton,
36+
"mActionSetToCanvasExtent.svg",
37+
"Use Current View Extent",
3938
)
39+
self._style_tool_button(self.drawOnMapButton, "mActionAddBasicRectangle.svg", "Draw on Map")
4040
self.drawOnMapButton.setCheckable(True)
4141
self.drawOnMapButton.clicked.connect(self.drawOnMap)
4242
self._draw_extent_tool = None

loopstructural/gui/visualisation/feature_list_widget.py

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,7 @@ def __init__(self, parent=None, *, model_manager=None, viewer=None, data_manager
5656
self.data_manager = data_manager
5757

5858
# Add buttons
59-
self.addBoundingBoxButton = self._make_tool_button(
60-
"extents.svg", "Add Model Bounding Box"
61-
)
59+
self.addBoundingBoxButton = self._make_tool_button("extents.svg", "Add Model Bounding Box")
6260
self.addFaultSurfacesButton = self._make_custom_icon_tool_button(
6361
"fault.svg", "Add Fault Surfaces"
6462
)
@@ -724,7 +722,12 @@ def _extract_line_xy(self, layer) -> Optional[np.ndarray]:
724722
except Exception:
725723
target_crs = None
726724
source_crs = layer.sourceCrs()
727-
if target_crs is not None and target_crs.isValid() and source_crs.isValid() and source_crs != target_crs:
725+
if (
726+
target_crs is not None
727+
and target_crs.isValid()
728+
and source_crs.isValid()
729+
and source_crs != target_crs
730+
):
728731
geom = QgsGeometry(geom)
729732
geom.transform(QgsCoordinateTransform(source_crs, target_crs, QgsProject.instance()))
730733

loopstructural/main/m2l_api.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,9 @@ def extract_basal_contacts(
9898
unit_name_col = 'UNITNAME' if 'UNITNAME' in geology.columns else unit_name_field
9999
if unit_name_col and unit_name_col in geology.columns:
100100
geology_unit_names = {str(v).strip() for v in geology[unit_name_col].dropna().unique()}
101-
stratigraphic_names = {str(name).strip() for name in stratigraphic_order if name is not None}
101+
stratigraphic_names = {
102+
str(name).strip() for name in stratigraphic_order if name is not None
103+
}
102104
ignored_names = {str(unit).strip() for unit in ignore_units if unit is not None}
103105
missing_from_column = sorted(geology_unit_names - stratigraphic_names - ignored_names)
104106
if missing_from_column:

loopstructural/main/model_manager.py

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -549,11 +549,9 @@ def update_foliation_features(self):
549549
data = []
550550
groupname = group.name
551551
stratigraphic_column[groupname] = {}
552-
for u in reversed(group.units):
552+
for u in group.units:
553553
unit_data = self.stratigraphy.get(u.name, None)
554-
if unit_data is None:
555-
continue
556-
else:
554+
if unit_data is not None:
557555
if 'contact' in unit_data:
558556
contact = unit_data['contact']
559557
if not contact.empty:
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
"""Regression test for the training-value / isovalue direction bug.
2+
3+
`GeologicalModelManager.update_foliation_features` assigns a scalar `val` to
4+
each unit's basal contact before handing the data to the interpolator.
5+
`StratigraphicColumn.get_isovalues` (LoopStructural core) later decides which
6+
name to stamp on each extracted isosurface, using its own idea of which
7+
value belongs to which unit.
8+
9+
10+
Both walk `reversed(group.units)`, accumulating cumulative thickness the
11+
same way, so a unit's own training value must equal `u.min()` -- the
12+
cumulative thickness *before* that unit's own thickness is added. This is
13+
also each unit's true base: `add_unit(..., where='top')` (the default)
14+
appends to the end of the column, so building a column correctly means
15+
adding the truly oldest unit first and progressively younger ones after --
16+
each unit's own base is the boundary shared with the next-older neighbour
17+
processed just before it, i.e. `min()`. See LoopStructural's own
18+
`test_get_isovalues_multi_unit_group` (`tests/unit/modelling/
19+
test_stratigraphic_column.py`), whose comment states this explicitly: "the
20+
base of the oldest unit in a group is 0".
21+
22+
If training and `get_isovalues()` disagree on this, every extracted surface
23+
gets labelled with the wrong unit while keeping correct geometry -- see the
24+
"stratigraphic column was reversed" fixes in model_manager.py (2025-07-21)
25+
and the widget (2025-08-21, reverted 2025-09-08). This has flipped back and
26+
forth as this plugin and LoopStructural evolved independently; this test
27+
pins the invariant so a future change on either side fails loudly here
28+
instead of silently inverting a user's model.
29+
30+
Note this is a separate concern from whether a stratigraphic column's units
31+
were themselves *added* in the correct oldest-to-youngest order -- if they
32+
weren't, `min()`/`max()` stop corresponding to true geological base/top no
33+
matter what training does, and the fix is to reorder the column's units,
34+
not to change which value training uses.
35+
"""
36+
37+
import pandas as pd
38+
import pytest
39+
from LoopStructural import StratigraphicColumn
40+
41+
from loopstructural.main.model_manager import GeologicalModelManager
42+
43+
44+
def _contact(unit_name):
45+
"""A minimal single-point basal contact, tagged with its unit name so
46+
the test can recover which row came from which unit after the group
47+
DataFrames get concatenated."""
48+
return pd.DataFrame({'X': [0.0], 'Y': [0.0], 'Z': [0.0], 'source_unit': [unit_name]})
49+
50+
51+
@pytest.fixture
52+
def manager(monkeypatch):
53+
manager = GeologicalModelManager()
54+
55+
captured_calls = []
56+
57+
def fake_create_and_add_foliation(name, data=None, **kwargs):
58+
captured_calls.append(data)
59+
return object() # stand-in foliation, only passed back into add_unconformity
60+
61+
monkeypatch.setattr(manager.model, 'create_and_add_foliation', fake_create_and_add_foliation)
62+
monkeypatch.setattr(manager.model, 'add_unconformity', lambda *a, **k: None)
63+
manager._captured_calls = captured_calls
64+
return manager
65+
66+
67+
class TestTrainingValueMatchesIsovalue:
68+
def test_single_group_three_units(self, manager):
69+
column = StratigraphicColumn()
70+
column.clear(basement=False) # single flat group, no unconformities
71+
column.add_unit(name='oldest', thickness=100.0, where='top')
72+
column.add_unit(name='middle', thickness=200.0, where='top')
73+
column.add_unit(name='youngest', thickness=300.0, where='top')
74+
75+
manager.stratigraphic_column = column
76+
for name in ('oldest', 'middle', 'youngest'):
77+
manager.stratigraphy[name]['contact'] = _contact(name)
78+
79+
manager.update_foliation_features()
80+
81+
training_values = self._training_values_by_unit(manager._captured_calls)
82+
expected_values = {name: entry['value'] for name, entry in column.get_isovalues().items()}
83+
84+
for unit_name in ('oldest', 'middle', 'youngest'):
85+
assert training_values[unit_name] == pytest.approx(expected_values[unit_name]), (
86+
f"'{unit_name}' was trained with val={training_values[unit_name]} but "
87+
f"get_isovalues() will label the value={expected_values[unit_name]} surface "
88+
f"with this unit's name -- the trained field and the isosurface labels "
89+
f"disagree on direction, so extracted surfaces will get the wrong unit name."
90+
)
91+
92+
def test_two_groups_split_by_unconformity(self, manager):
93+
column = StratigraphicColumn()
94+
column.clear(basement=False)
95+
column.add_unit(name='basin_floor', thickness=50.0, where='top')
96+
column.add_unit(name='basin_fill', thickness=150.0, where='top')
97+
column.add_unconformity(name='regional_unconformity', where='top')
98+
column.add_unit(name='cover_lower', thickness=80.0, where='top')
99+
column.add_unit(name='cover_upper', thickness=120.0, where='top')
100+
101+
manager.stratigraphic_column = column
102+
for name in ('basin_floor', 'basin_fill', 'cover_lower', 'cover_upper'):
103+
manager.stratigraphy[name]['contact'] = _contact(name)
104+
105+
manager.update_foliation_features()
106+
107+
training_values = self._training_values_by_unit(manager._captured_calls)
108+
expected_values = {name: entry['value'] for name, entry in column.get_isovalues().items()}
109+
110+
for unit_name in ('basin_floor', 'basin_fill', 'cover_lower', 'cover_upper'):
111+
assert training_values[unit_name] == pytest.approx(expected_values[unit_name])
112+
113+
def test_undigitised_unit_does_not_shift_later_units_in_group(self, manager):
114+
"""Regression test for a real bug: a unit with no digitised contact
115+
or orientation data (e.g. a "Top" unit nobody has mapped points for)
116+
must still contribute its own thickness to `val` for every unit
117+
that follows it in the group -- `update_foliation_features` used to
118+
`continue` past an undigitised unit before accumulating its
119+
thickness, which shifted every later unit's trained value relative
120+
to what `get_isovalues()` expects.
121+
"""
122+
column = StratigraphicColumn()
123+
column.clear(basement=False)
124+
column.add_unit(name='basin_floor', thickness=50.0, where='top')
125+
column.add_unit(name='basin_fill', thickness=150.0, where='top')
126+
column.add_unit(name='Top', thickness=999.0, where='top')
127+
128+
manager.stratigraphic_column = column
129+
for name in ('basin_floor', 'basin_fill'):
130+
manager.stratigraphy[name]['contact'] = _contact(name)
131+
# 'Top' deliberately has no entry in manager.stratigraphy at all.
132+
133+
manager.update_foliation_features()
134+
135+
training_values = self._training_values_by_unit(manager._captured_calls)
136+
expected_values = {name: entry['value'] for name, entry in column.get_isovalues().items()}
137+
138+
for unit_name in ('basin_floor', 'basin_fill'):
139+
assert training_values[unit_name] == pytest.approx(expected_values[unit_name]), (
140+
f"'{unit_name}' was trained with val={training_values[unit_name]} but "
141+
f"get_isovalues() expects value={expected_values[unit_name]} -- an "
142+
f"undigitised unit earlier in the group must still shift later units' "
143+
f"trained values by its own thickness."
144+
)
145+
146+
@staticmethod
147+
def _training_values_by_unit(captured_calls):
148+
combined = pd.concat(captured_calls, ignore_index=True)
149+
return dict(zip(combined['source_unit'], combined['val']))

0 commit comments

Comments
 (0)