Repository navigation
test: pin the recipe and inequality-constraint problems (strict xfail) - #374
Draft
lachlangrose wants to merge 1 commit into
Draft
lachlangrose wants to merge 1 commit into
lachlangrose wants to merge 1 commit into
Conversation
Recipe: to_recipe_json / from_recipe_json do not store a feature's build parameters (nelements, interpolatortype, regularisation), so a model built with anything but the defaults rebuilds with the defaults and gives a different field (differences of 12 to 60 on a field of range about 560). The default-parameter case passes; the three others are strict xfails. Inequality: ADMM enforces the bounds with the arguments the existing builder test uses, but with its default arguments returns essentially the unconstrained solution (4 of 24 bounds violated, worst 0.056), and with solver cg the inequality data are dropped without a warning. A control (the unconstrained solution does violate bounds) and the working ADMM configuration pass; the default-ADMM case and the cg-warning expectation are strict xfails. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This was referenced Oct 29, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tests that pin two problems I found while looking for examples to write (#373). Both are strict xfails, so the suite stays green and they fail loudly (XPASS) when the problem is fixed. Independent of the other open PRs (based on
master).1. Recipes do not store a feature's build parameters
tests/unit/modelling/test_recipe_reproduces_field.pyGeologicalModel.to_recipe_json/from_recipe_jsonare provisional API. The existing recipe tests check feature names, the data and the stratigraphic column, but not the field. A feature entry in the recipe contains onlyname,kind,faultsandregions, so the build parameters are lost and the rebuilt model uses the defaults.nelements=2000interpolatortype="PLI"regularisation=10API.mdrequires a user-facing example before a provisional symbol can be promoted to stable. An example now would imply the recipe reproduces a model, which is only true for defaults.Diagnosis (
LoopStructural/modelling/core/_model_serializer.py)The build parameters are only the first of several problems. All were found by running a round trip.
to_recipe_dict): a feature entry isname,kind,faultsand a hardcodedregions: []. Nothing else.from_recipe_dict): ignoreskind. Every entry is rebuilt withcreate_and_add_foliation, so a fault comes back as a plain foliation (in a small fault model the strata change by 0.21 on a field of range 10 and fault displacement by 0.49).feature_recipe_kind: testsisinstance(..., GeologicalFeature)andStructuralFramebefore the subclasses, so a fault is written asstructural_frame(the"fault"branch is unreachable,FaultSegmentsubclassesStructuralFrame) and an unconformity asfoliation.from_recipe_jsonraisesValueError: Could not recreate feature '__strati_unconformity'.group_mappingof the stratigraphic column is lost ({'Group_0': 'strati'}becomes{}); unit order survives.data=(not taken frommodel.data) is rebuilt frommodel.datafiltered byfeature_name: a half-size subset gives a difference of 20.5.feature.builder.build_argumentsholdsregularisation,damp,solver,tol, etc., but itsnelementsis the realised element count (17,732 for a request of 10,000), andinterpolatortypeis not in it (it is on the interpolator). Replayingbuild_argumentsas-is is wrong (differences 6 to 25); replaying them with the originally requestednelementsreproduces the field exactly (0.0 in all four cases I tried). The requestednelementsis not retained anywhere today.Not tested: fold frames, folded foliations (their fold functions are callables, which JSON cannot hold), intrusions, domain faults.
Suggested fix (not in this PR)
All creation funnels through
GeologicalModel.create_and_add_feature(feature_type, name, **params), so record the requested JSON-safe parameters there (interpolatortype,nelements,tol,regularisation,solver, ...), write them in the recipe, and have the reader replaycreate_and_add_feature(kind, name, **params)in order, resolving faults by name. Also writegroup_mapping. Until a kind is supported,to_recipe_dictshould raise a clear error rather than write an entry that rebuilds wrongly.2. Inequality constraints
packages/loop_interpolation/tests/test_inequality_enforcement.pyDocumented meaning:
lower <= f(x, y, z) <= upper. The scenario is a sparse dome model (3 contacts, 2 orientations) with 24 one-sided bounds on the 0.5 horizon that the data alone get wrong 4 times.nmajor=10, admm_weight=0.01, maxiter=50(the existing builder test's arguments) enforces every boundcg, supplying inequality data warns that they are not usedThe
cgtest is an expectation, not a documented contract, and the reason string says so. For the inequality problem the cause is not diagnosed: I only know that ADMM's result depends on its arguments (using only some of the three above also leaves the bounds violated). I checked thatcaplogdoes capture this library's log records, so thecgtest can XPASS once a warning exists.Correction
My earlier note on #373 said inequality constraints "did not change the solution" with ADMM. That was only for default arguments; I had also used a different call order from the existing test. With the existing test's flow and arguments they do work, as the passing test above shows.
Checks
Each xfail was run with
--runxfailand fails for the stated reason (numbers above). ruff clean.🤖 Generated with Claude Code