From ed17d931a04efd574eb6dec0abe4903dddc683b0 Mon Sep 17 00:00:00 2001 From: lachlangrose Date: Thu, 20 Aug 2026 14:30:37 +0930 Subject: [PATCH 1/4] 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 --- .../test_stratigraphic_value_consistency.py | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 tests/qgis/test_stratigraphic_value_consistency.py diff --git a/tests/qgis/test_stratigraphic_value_consistency.py b/tests/qgis/test_stratigraphic_value_consistency.py new file mode 100644 index 0000000..5135a20 --- /dev/null +++ b/tests/qgis/test_stratigraphic_value_consistency.py @@ -0,0 +1,102 @@ +"""Regression test for the training-value / isovalue direction bug. + +`GeologicalModelManager.update_foliation_features` assigns a scalar `val` to +each unit's basal contact before handing the data to the interpolator. +`StratigraphicColumn.get_isovalues` (LoopStructural core) later decides which +name to stamp on each extracted isosurface, using its own idea of which +value belongs to which unit. + +These two must agree on direction (does value increase from oldest-to- +youngest, or youngest-to-oldest?), or every extracted surface gets labelled +with the wrong unit while keeping correct geometry -- see the "stratigraphic +column was reversed" fixes in model_manager.py (2025-07-21) and the widget +(2025-08-21, reverted 2025-09-08). This has flipped back and forth as this +plugin and LoopStructural evolved independently; this test pins the +invariant so a future change on either side fails loudly here instead of +silently inverting a user's model. +""" + +import pandas as pd +import pytest +from LoopStructural import StratigraphicColumn + +from loopstructural.main.model_manager import GeologicalModelManager + + +def _contact(unit_name): + """A minimal single-point basal contact, tagged with its unit name so + the test can recover which row came from which unit after the group + DataFrames get concatenated.""" + return pd.DataFrame({'X': [0.0], 'Y': [0.0], 'Z': [0.0], 'source_unit': [unit_name]}) + + +@pytest.fixture +def manager(monkeypatch): + manager = GeologicalModelManager() + + captured_calls = [] + + def fake_create_and_add_foliation(name, data=None, **kwargs): + captured_calls.append(data) + return object() # stand-in foliation, only passed back into add_unconformity + + monkeypatch.setattr(manager.model, 'create_and_add_foliation', fake_create_and_add_foliation) + monkeypatch.setattr(manager.model, 'add_unconformity', lambda *a, **k: None) + manager._captured_calls = captured_calls + return manager + + +class TestTrainingValueMatchesIsovalue: + def test_single_group_three_units(self, manager): + column = StratigraphicColumn() + column.clear(basement=False) # single flat group, no unconformities + column.add_unit(name='oldest', thickness=100.0, where='top') + column.add_unit(name='middle', thickness=200.0, where='top') + column.add_unit(name='youngest', thickness=300.0, where='top') + + manager.stratigraphic_column = column + for name in ('oldest', 'middle', 'youngest'): + manager.stratigraphy[name]['contact'] = _contact(name) + + manager.update_foliation_features() + + training_values = self._training_values_by_unit(manager._captured_calls) + expected_values = { + name: entry['value'] for name, entry in column.get_isovalues().items() + } + + for unit_name in ('oldest', 'middle', 'youngest'): + assert training_values[unit_name] == pytest.approx(expected_values[unit_name]), ( + f"'{unit_name}' was trained with val={training_values[unit_name]} but " + f"get_isovalues() will label the value={expected_values[unit_name]} surface " + f"with this unit's name -- the trained field and the isosurface labels " + f"disagree on direction, so extracted surfaces will get the wrong unit name." + ) + + def test_two_groups_split_by_unconformity(self, manager): + column = StratigraphicColumn() + column.clear(basement=False) + column.add_unit(name='basin_floor', thickness=50.0, where='top') + column.add_unit(name='basin_fill', thickness=150.0, where='top') + column.add_unconformity(name='regional_unconformity', where='top') + column.add_unit(name='cover_lower', thickness=80.0, where='top') + column.add_unit(name='cover_upper', thickness=120.0, where='top') + + manager.stratigraphic_column = column + for name in ('basin_floor', 'basin_fill', 'cover_lower', 'cover_upper'): + manager.stratigraphy[name]['contact'] = _contact(name) + + manager.update_foliation_features() + + training_values = self._training_values_by_unit(manager._captured_calls) + expected_values = { + name: entry['value'] for name, entry in column.get_isovalues().items() + } + + for unit_name in ('basin_floor', 'basin_fill', 'cover_lower', 'cover_upper'): + assert training_values[unit_name] == pytest.approx(expected_values[unit_name]) + + @staticmethod + def _training_values_by_unit(captured_calls): + combined = pd.concat(captured_calls, ignore_index=True) + return dict(zip(combined['source_unit'], combined['val'])) From b637a74b71b7734c12baeba503aa08af3f15af47 Mon Sep 17 00:00:00 2001 From: lachlangrose Date: Mon, 24 Aug 2026 12:54:20 +0930 Subject: [PATCH 2/4] test: add regression test for undigitised unit affecting later units' trained values --- .../test_stratigraphic_value_consistency.py | 70 ++++++++++++++++--- 1 file changed, 62 insertions(+), 8 deletions(-) diff --git a/tests/qgis/test_stratigraphic_value_consistency.py b/tests/qgis/test_stratigraphic_value_consistency.py index 5135a20..0ae6ef7 100644 --- a/tests/qgis/test_stratigraphic_value_consistency.py +++ b/tests/qgis/test_stratigraphic_value_consistency.py @@ -6,14 +6,32 @@ name to stamp on each extracted isosurface, using its own idea of which value belongs to which unit. -These two must agree on direction (does value increase from oldest-to- -youngest, or youngest-to-oldest?), or every extracted surface gets labelled -with the wrong unit while keeping correct geometry -- see the "stratigraphic -column was reversed" fixes in model_manager.py (2025-07-21) and the widget -(2025-08-21, reverted 2025-09-08). This has flipped back and forth as this -plugin and LoopStructural evolved independently; this test pins the -invariant so a future change on either side fails loudly here instead of -silently inverting a user's model. + +Both walk `reversed(group.units)`, accumulating cumulative thickness the +same way, so a unit's own training value must equal `u.min()` -- the +cumulative thickness *before* that unit's own thickness is added. This is +also each unit's true base: `add_unit(..., where='top')` (the default) +appends to the end of the column, so building a column correctly means +adding the truly oldest unit first and progressively younger ones after -- +each unit's own base is the boundary shared with the next-older neighbour +processed just before it, i.e. `min()`. See LoopStructural's own +`test_get_isovalues_multi_unit_group` (`tests/unit/modelling/ +test_stratigraphic_column.py`), whose comment states this explicitly: "the +base of the oldest unit in a group is 0". + +If training and `get_isovalues()` disagree on this, every extracted surface +gets labelled with the wrong unit while keeping correct geometry -- see the +"stratigraphic column was reversed" fixes in model_manager.py (2025-07-21) +and the widget (2025-08-21, reverted 2025-09-08). This has flipped back and +forth as this plugin and LoopStructural evolved independently; this test +pins the invariant so a future change on either side fails loudly here +instead of silently inverting a user's model. + +Note this is a separate concern from whether a stratigraphic column's units +were themselves *added* in the correct oldest-to-youngest order -- if they +weren't, `min()`/`max()` stop corresponding to true geological base/top no +matter what training does, and the fix is to reorder the column's units, +not to change which value training uses. """ import pandas as pd @@ -96,6 +114,42 @@ def test_two_groups_split_by_unconformity(self, manager): for unit_name in ('basin_floor', 'basin_fill', 'cover_lower', 'cover_upper'): assert training_values[unit_name] == pytest.approx(expected_values[unit_name]) + + def test_undigitised_unit_does_not_shift_later_units_in_group(self, manager): + """Regression test for a real bug: a unit with no digitised contact + or orientation data (e.g. a "Top" unit nobody has mapped points for) + must still contribute its own thickness to `val` for every unit + that follows it in the group -- `update_foliation_features` used to + `continue` past an undigitised unit before accumulating its + thickness, which shifted every later unit's trained value relative + to what `get_isovalues()` expects. + """ + column = StratigraphicColumn() + column.clear(basement=False) + column.add_unit(name='basin_floor', thickness=50.0, where='top') + column.add_unit(name='basin_fill', thickness=150.0, where='top') + column.add_unit(name='Top', thickness=999.0, where='top') + + manager.stratigraphic_column = column + for name in ('basin_floor', 'basin_fill'): + manager.stratigraphy[name]['contact'] = _contact(name) + # 'Top' deliberately has no entry in manager.stratigraphy at all. + + manager.update_foliation_features() + + training_values = self._training_values_by_unit(manager._captured_calls) + expected_values = { + name: entry['value'] for name, entry in column.get_isovalues().items() + } + + for unit_name in ('basin_floor', 'basin_fill'): + assert training_values[unit_name] == pytest.approx(expected_values[unit_name]), ( + f"'{unit_name}' was trained with val={training_values[unit_name]} but " + f"get_isovalues() expects value={expected_values[unit_name]} -- an " + f"undigitised unit earlier in the group must still shift later units' " + f"trained values by its own thickness." + ) + @staticmethod def _training_values_by_unit(captured_calls): combined = pd.concat(captured_calls, ignore_index=True) From 04b4516d5dbf99f334bb099dbe219e92ff594731 Mon Sep 17 00:00:00 2001 From: lachlangrose Date: Mon, 24 Aug 2026 13:18:18 +0930 Subject: [PATCH 3/4] style: fix black formatting and ruff lint (unnecessary generator) Co-Authored-By: Claude Sonnet 5 --- .../gui/map2loop_tools/fault_topology_widget.py | 6 ++---- .../gui/modelling/model_definition/bounding_box.py | 8 ++++---- .../gui/visualisation/feature_list_widget.py | 11 +++++++---- loopstructural/main/m2l_api.py | 4 +++- tests/qgis/test_stratigraphic_value_consistency.py | 13 +++---------- 5 files changed, 19 insertions(+), 23 deletions(-) diff --git a/loopstructural/gui/map2loop_tools/fault_topology_widget.py b/loopstructural/gui/map2loop_tools/fault_topology_widget.py index 5d786ac..76fa7ff 100644 --- a/loopstructural/gui/map2loop_tools/fault_topology_widget.py +++ b/loopstructural/gui/map2loop_tools/fault_topology_widget.py @@ -174,7 +174,7 @@ def _run_topology(self): # not just the ones map2loop found a relationship for. A fault with # no detected topological relationship is still a real fault and # must not be dropped from the fault topology. - new_faults = set(str(v) for v in gdf['ID'].unique()) + new_faults = {str(v) for v in gdf['ID'].unique()} # Add new faults; never remove existing ones here, so faults # without a detected relationship (or ones the user added @@ -209,9 +209,7 @@ def _run_topology(self): else: f1 = str(row.iloc[0]) f2 = str(row.iloc[1]) - ft.update_fault_relationship( - f1, f2, FaultRelationshipType.ABUTTING - ) + ft.update_fault_relationship(f1, f2, FaultRelationshipType.ABUTTING) except Exception: pass diff --git a/loopstructural/gui/modelling/model_definition/bounding_box.py b/loopstructural/gui/modelling/model_definition/bounding_box.py index d5062f4..4c594f6 100644 --- a/loopstructural/gui/modelling/model_definition/bounding_box.py +++ b/loopstructural/gui/modelling/model_definition/bounding_box.py @@ -32,11 +32,11 @@ def __init__(self, parent=None, data_manager=None): self.selectFromCurrentLayerButton, "mActionZoomToLayer.svg", "Select from Current Layer" ) self._style_tool_button( - self.useCurrentViewExtentButton, "mActionSetToCanvasExtent.svg", "Use Current View Extent" - ) - self._style_tool_button( - self.drawOnMapButton, "mActionAddBasicRectangle.svg", "Draw on Map" + self.useCurrentViewExtentButton, + "mActionSetToCanvasExtent.svg", + "Use Current View Extent", ) + self._style_tool_button(self.drawOnMapButton, "mActionAddBasicRectangle.svg", "Draw on Map") self.drawOnMapButton.setCheckable(True) self.drawOnMapButton.clicked.connect(self.drawOnMap) self._draw_extent_tool = None diff --git a/loopstructural/gui/visualisation/feature_list_widget.py b/loopstructural/gui/visualisation/feature_list_widget.py index e6e6539..90c17a2 100644 --- a/loopstructural/gui/visualisation/feature_list_widget.py +++ b/loopstructural/gui/visualisation/feature_list_widget.py @@ -56,9 +56,7 @@ def __init__(self, parent=None, *, model_manager=None, viewer=None, data_manager self.data_manager = data_manager # Add buttons - self.addBoundingBoxButton = self._make_tool_button( - "extents.svg", "Add Model Bounding Box" - ) + self.addBoundingBoxButton = self._make_tool_button("extents.svg", "Add Model Bounding Box") self.addFaultSurfacesButton = self._make_custom_icon_tool_button( "fault.svg", "Add Fault Surfaces" ) @@ -724,7 +722,12 @@ def _extract_line_xy(self, layer) -> Optional[np.ndarray]: except Exception: target_crs = None source_crs = layer.sourceCrs() - if target_crs is not None and target_crs.isValid() and source_crs.isValid() and source_crs != target_crs: + if ( + target_crs is not None + and target_crs.isValid() + and source_crs.isValid() + and source_crs != target_crs + ): geom = QgsGeometry(geom) geom.transform(QgsCoordinateTransform(source_crs, target_crs, QgsProject.instance())) diff --git a/loopstructural/main/m2l_api.py b/loopstructural/main/m2l_api.py index 2b5f1d8..82aa0a8 100644 --- a/loopstructural/main/m2l_api.py +++ b/loopstructural/main/m2l_api.py @@ -98,7 +98,9 @@ def extract_basal_contacts( unit_name_col = 'UNITNAME' if 'UNITNAME' in geology.columns else unit_name_field if unit_name_col and unit_name_col in geology.columns: geology_unit_names = {str(v).strip() for v in geology[unit_name_col].dropna().unique()} - stratigraphic_names = {str(name).strip() for name in stratigraphic_order if name is not None} + stratigraphic_names = { + str(name).strip() for name in stratigraphic_order if name is not None + } ignored_names = {str(unit).strip() for unit in ignore_units if unit is not None} missing_from_column = sorted(geology_unit_names - stratigraphic_names - ignored_names) if missing_from_column: diff --git a/tests/qgis/test_stratigraphic_value_consistency.py b/tests/qgis/test_stratigraphic_value_consistency.py index 0ae6ef7..e127339 100644 --- a/tests/qgis/test_stratigraphic_value_consistency.py +++ b/tests/qgis/test_stratigraphic_value_consistency.py @@ -79,9 +79,7 @@ def test_single_group_three_units(self, manager): manager.update_foliation_features() training_values = self._training_values_by_unit(manager._captured_calls) - expected_values = { - name: entry['value'] for name, entry in column.get_isovalues().items() - } + expected_values = {name: entry['value'] for name, entry in column.get_isovalues().items()} for unit_name in ('oldest', 'middle', 'youngest'): assert training_values[unit_name] == pytest.approx(expected_values[unit_name]), ( @@ -107,14 +105,11 @@ def test_two_groups_split_by_unconformity(self, manager): manager.update_foliation_features() training_values = self._training_values_by_unit(manager._captured_calls) - expected_values = { - name: entry['value'] for name, entry in column.get_isovalues().items() - } + expected_values = {name: entry['value'] for name, entry in column.get_isovalues().items()} for unit_name in ('basin_floor', 'basin_fill', 'cover_lower', 'cover_upper'): assert training_values[unit_name] == pytest.approx(expected_values[unit_name]) - def test_undigitised_unit_does_not_shift_later_units_in_group(self, manager): """Regression test for a real bug: a unit with no digitised contact or orientation data (e.g. a "Top" unit nobody has mapped points for) @@ -138,9 +133,7 @@ def test_undigitised_unit_does_not_shift_later_units_in_group(self, manager): manager.update_foliation_features() training_values = self._training_values_by_unit(manager._captured_calls) - expected_values = { - name: entry['value'] for name, entry in column.get_isovalues().items() - } + expected_values = {name: entry['value'] for name, entry in column.get_isovalues().items()} for unit_name in ('basin_floor', 'basin_fill'): assert training_values[unit_name] == pytest.approx(expected_values[unit_name]), ( From 3f64f742f5fc5a790599d0c1c63dc972d8db7de4 Mon Sep 17 00:00:00 2001 From: lachlangrose Date: Mon, 24 Aug 2026 14:36:37 +0930 Subject: [PATCH 4/4] 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 --- loopstructural/main/model_manager.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/loopstructural/main/model_manager.py b/loopstructural/main/model_manager.py index 779689f..4bea402 100644 --- a/loopstructural/main/model_manager.py +++ b/loopstructural/main/model_manager.py @@ -549,11 +549,9 @@ def update_foliation_features(self): data = [] groupname = group.name stratigraphic_column[groupname] = {} - for u in reversed(group.units): + for u in group.units: unit_data = self.stratigraphy.get(u.name, None) - if unit_data is None: - continue - else: + if unit_data is not None: if 'contact' in unit_data: contact = unit_data['contact'] if not contact.empty: