docs(cif): CIF output correctness design doc (#68, #238)#303
Open
manzuoni-astera wants to merge 1 commit into
Open
docs(cif): CIF output correctness design doc (#68, #238)#303manzuoni-astera wants to merge 1 commit into
manzuoni-astera wants to merge 1 commit into
Conversation
…se#238) Consolidates the CIF-output issues Marcus asked to catalog: per-model chain-ID handling (RF3/Boltz preserve, Protenix reinvents), the entity/header gaps in the native writers, and the silent multi-chain remap hazard in patch_output_cif_files. Documents that atom_site.id uniqueness (#68a) is already satisfied by the pinned biotite 1.4.0 (verified), narrowing diff-use#68 scope to entities + header, and lays out a phased plan toward retiring the post-processing patch. Refs diff-use#68, diff-use#238.
manzuoni-astera
requested review from
a team,
Abdelsalam-Abbas and
denis-zaitsev
as code owners
July 8, 2026 17:44
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new design/RFC document, ChangesDesign Documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 8, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a design/RFC document to catalog current mmCIF output correctness gaps across model wrappers (RF3/Boltz/Protenix), relate them to #68 and #238, and propose a phased plan to remove reliance on scripts/patch_output_cif_files.py while avoiding silent multi-chain evaluation corruption.
Changes:
- Introduces a new design doc describing the current CIF write path and requirement status for
atom_site.id, entities, and header carry-over. - Documents the Protenix chain-ID mismatch root cause and the specific silent-corruption hazard in
patch_output_cif_files.pywhen the guard is disabled. - Proposes a phased remediation plan (header carry-over → Protenix chain IDs → entity generation → retire patch) with cited code references.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+31
| | **RF3** | `InferenceInput.from_atom_array(atom_array, chain_info=…)`; `model_aa = pipeline_output["atom_array"].copy()` (`models/rf3/wrapper.py:340,402`) | **Preserved** from the input AtomArray — no lossy round-trip | | ||
| | **Boltz** | YAML carries per-chain `id: {chain_id}` (`models/boltz/wrapper.py:479,493`); echoed back and decoded (`models/boltz/wrapper.py:207`) | **Preserved** | | ||
| | **Protenix** | Structure → chain-less JSON → `SampleDictToFeatures(json).get_feature_dict()` rebuilds the AtomArray (`models/protenix/wrapper.py:393`) | **Reinvented** (A, B, C… by entity/copy order) | |
Comment on lines
+33
to
+37
| Protenix is the outlier: `structure_to_protenix_json()` (`models/protenix/structure_processing.py:515`) | ||
| builds an AlphaFold3-style JSON whose entity dicts have **no per-chain `id` slot** — chains | ||
| are collapsed into entities with a copy `count` (`structure_processing.py:660`). The real | ||
| chain IDs are only set on the *input-side* array (`structure_processing.py:566`, | ||
| `set_annotation("label_asym_id", …)`), which is discarded; the array that reaches output |
Comment on lines
+38
to
+40
| (`models/protenix/wrapper.py:478` `model_aa = cast(AtomArray, atom_array_protenix)`) has | ||
| Protenix's invented letters. That array is what gets written to `refined.cif` | ||
| (`utils/guidance_script_utils.py:325-342`). |
Comment on lines
+46
to
+50
| - `save_everything` → `refined.cif`: `utils/guidance_script_utils.py:339-342` (ensemble becomes a | ||
| multi-model `AtomArrayStack` at `:331-337`). | ||
| - `save_structure_to_cif`: `utils/atom_array_utils.py:183-185` (generic; used by synthetic-density | ||
| and altloc-fixed temp CIFs). | ||
| - Trajectory CIFs: `utils/guidance_script_utils.py:123,148` via biotite `save_structure` (a |
Comment on lines
+53
to
+54
| The input structure is read with atomworks `parse(...)` (`guidance_script_utils.py:236`), which | ||
| returns only the `AtomArray` — **the input CIF's header categories are dropped before writing.** |
Comment on lines
+75
to
+77
| into the template so its **header is preserved** (`:233`) → reconcile `pdbx_poly_seq_scheme` | ||
| → overwrite `atom_site.id` with a unique `arange` (`:245`) → ensure `occupancy`/`B_iso` | ||
| columns → write `<stem>-patched.cif`. |
Comment on lines
+123
to
+127
| correspondence (`eval/structure_utils.py:191`) before writing, then re-enable the | ||
| `patch_output_cif_files.py` guard for those cases. Caveat: the reconciler keys on **sorted | ||
| chain position**, not label (`utils/atom_array_utils.py:604-635`, | ||
| `utils/atom_reconciler.py:64-107`), so this is only correct when Protenix's sorted chain | ||
| order matches the reference's. |
Comment on lines
+153
to
+156
| - Protenix chain loss: `models/protenix/structure_processing.py:515,566,660`; `models/protenix/wrapper.py:393,478,505` | ||
| - Chain-ID preservation (RF3/Boltz): `models/rf3/wrapper.py:340,402`; `models/boltz/wrapper.py:207,479,493` | ||
| - Writers: `utils/guidance_script_utils.py:236,325-342,123,148`; `utils/atom_array_utils.py:183-185` | ||
| - Reconciler / position-based matching: `eval/structure_utils.py:191`; `utils/atom_reconciler.py:64-107`; `utils/atom_array_utils.py:604-635` |
Comment on lines
+101
to
+103
| 3. **No cross-model entity bookkeeping** — only Protenix carries `label_entity_id` | ||
| (`models/protenix/structure_processing.py:241,281,340`); Boltz/RF3 do not, so there is no | ||
| uniform place to build `entity`/`entity_poly`. |
Comment on lines
+61
to
+62
| | **(b) entity / `entity_id`** | **Missing.** No `entity`/`entity_poly` category is ever constructed; `set_structure` only emits `label_entity_id` if the array carries that annotation. | `guidance_script_utils.py:339-342`; patch injects it at `patch_output_cif_files.py:220-231` | | ||
| | **(c) header (cell, symmetry, struct, entity_poly, pdbx_poly_seq_scheme, …)** | **Missing.** Only the custom `sampleworks` metadata block is written; everything else was dropped at parse time. | `guidance_script_utils.py:341`; patch recovers header from the template at `patch_output_cif_files.py:233` | |
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.
Summary
Both #68 and #238 are facets of the same "fix our CIF outputs" problem, and Marcus asked (on #238) for a design doc to catalog and consolidate the work before we start hacking at it. This adds
docs/design/cif-output-correctness.md.I investigated the code first rather than guess — key findings:
idslot (structure_processing.py:515,660). Its real IDs are set only on the input-side array that gets discarded. This is the root cause the issue points at.return msgguard commented out inpatch_output_cif_files.py(from fix: quick patch to enforce that the ref chain is the one assigned to… #237), multi-chain outputs are remapped against the reference by positional zip — residues can land on the wrong chain with no error. That's the concrete eval hazard behind the P0.atom_site.idfor every row (including the multi-model ensemble case). So Fix CIF output files #68's "unique id" requirement is already met — the patch overwritesidonly because it edits a template CIF. The real remaining native gaps are entity ids and header carry-over.The doc lays out a phased plan (native header copy → Protenix chain-ID fix, scoped vs. general multi-chain → entity generation → retire the patch), with the cross-model differences tabulated and every claim cited to
file:line.This PR is docs-only — no code changes. It's the "catalog + plan" step; the phased fixes (and a Phase-0 regression test pinning
atom_site.iduniqueness against a biotite bump) are follow-ups once we agree the direction. Flagging the open questions in §7 for @marcuscollins.Refs #68, #238.
Summary by CodeRabbit