feature/SOF-8034 Fix: preserve the input crystal in build metadata - #297
Merged
Conversation
Builders recorded a conventionalized copy of the material instead of the one that was passed in, so a Total Energy job run on a primitive cell could not be reached from a slab built from it. Store the input untransformed and move the conventionalization to build time, where the use_conventional_cell flag already lives. - CrystalLatticePlanesBuilder._generate applies the transform - SlabConfiguration.from_parameters forwards the flag instead of rebinding - four helpers stop conventionalizing before building their configuration - create_atomic_layers pins the flag False, preserving its behaviour twisted is deliberately untouched: its analyzer reads atomic_layers.crystal as geometry without building, so the transform cannot reach it there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rewrite them the way the rest of the suite is written: call the helper and compare against a fixture, instead of asserting invariants between two invocations. - one parametrized test over the six builders, comparing the recorded crystal to the input material - create_atomic_layers pinned against a fixture like its siblings - drop the twisted test: that helper is unchanged from main, so it was testing code this branch does not touch 167 -> 73 lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
use_conventional_cell was already a no-op on the commensurate and grain-boundary paths before this branch -- the helper conventionalized, then from_parameters conventionalized again on the schema default. Documenting that in the docstrings of functions this branch only touches to delete a redundant transform is scope creep, and pointing code at a ticket number for it is worse. Those four files are now pure deletion. The dead parameter is recorded as a follow-up finding instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last of the five entry points. The rebind ran before the configuration was built, so the recorded crystal was the conventional cell rather than the input. Both SlabConfiguration.from_parameters calls below already forwarded use_conventional_cell, so removing the rebind is enough; the import goes with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…SOF-8034]" This reverts commit 40aae26.
Nothing in the suite caught the 8-atom -> 1-atom collapse when the transform is moved out of the twisted helper: the only existing twisted test uses graphene, whose conventional cell is itself, so conventionalizing there is a no-op. The regression was committed on this branch yesterday and caught by hand, not by CI. Pin the two scalars that move. Verified: re-applying the deletion fails this test and nothing else -- 1 failed, 310 passed. Also: do not recurse into an already-collected crystal (an input carrying build history would contribute the crystals nested inside it), and spell out lambda parameters per AGENTS.md HARD RULE 4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
VsevolodX
marked this pull request as ready for review
August 25, 2026 07:10
Comment-only; no code changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
timurbazhirov
approved these changes
Aug 26, 2026
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.
Closes the SOF-8034 defect: builders recorded a conventionalized copy of the material instead of the one passed in, so a Total Energy job run on a primitive cell was unreachable from a slab built from it.
create_slab(Ni_primitive, (0,0,1), 3)recordedad81bcd8…(conventional Ni4) instead of the input18d50c5c….What changed
The input crystal is now stored untransformed, and conventionalization moves to build time — where the
use_conventional_cellflag already lived.CrystalLatticePlanesBuilder._generateapplies the transformSlabConfiguration.from_parametersforwards the flag instead of rebindingmaterialcreate_atomic_layerspins the flagFalse, preserving its existing behaviourhash/scaledHashare computed fields and_idrides on the stored object, so no round-trip helper was needed — the ticket's root cause 3 turned out to be unnecessary rather than deferred.Two named exclusions — the criterion is not "every builder"
TwistedNanoribbonsInterfaceAnalyzerreadsatomic_layers.crystalas geometry and never calls the builder, so the transform cannot reach it there.twisted/helpers.pyis deliberately unchanged; an earlier revision of this branch did change it and silently collapseduse_conventional_cell=Truefrom 8 atoms to 1. Guarded by a test now.metadata.build == [], onmainand after this change alike. Nothing to inherit. Separate ticket.Consequences
from_parametersnever forwarded the flag, so every stored configuration reads"use_conventional_cell": true— including slabs built from the primitive cell._generateused to ignore it and now honors it, so those rebuild conventionalized (measured: 3 atoms a=2.479 → 12 atoms a=3.5058). Not fixable — a staletrueis indistinguishable from a real one. Pinned by a test. Rebuild such a slab from its crystal, not from its metadata.conventionalize²becomingconventionalize¹, not damage.atomic_layers.crystalnow see the input cell.Verification
313 passed(was303onmain; +10 new) with zero existing hash fixtures edited — that was the stop condition for geometry movementtests/py/unit/test_build_metadata_records_input_crystal.py:10 passed, and6 failed / 3 passedagainst unmodified sourcesurface_energy.featurepasses end to end against a wheel built from this branch, served through a local JupyterLite (verified by its access log)tb-reviewerrounds; round 3 approvedManual checks
Merging accepts anything left unticked.
ideal_crystal/builder.py:41andanalyze/interface/twisted_nanoribbons.py:48passuse_conventional_cell=False. Confirm their build metadata serializesfalse, not the schema defaulttrue.interface/base/builder.py:105-106derives the name from a reduced formula. Primitive Ni and Ni4 both reduce toNi— confirm one built ZSL interface'snameis byte-identical before and after.lattice.typeon the recorded crystal is now the input's. If the input fixture itself carriesTRIfor a cubic cell, that is upstream, not this task.Not done here
calculate_interfacial_distance_from_rdf'sisinstance(x, SlabConfiguration)branch returns 2.35 Å wheremainreturned 3.85 Å for primitive Si. It has no caller in made, api-examples or the notebooks, and 2.35 Å is what its docstring promises — recorded as a finding, either delete the dead branch or give it a caller and a test.🤖 Generated with Claude Code