Port and fix the UDM→ORD converter - #963
Draft
bdeadman wants to merge 15 commits into
Draft
Conversation
The udm-converter branch diverged too far from main to merge cleanly (main has since rewritten the ORM layer, message_helpers, and reshuffled ord_schema/scripts/). Port just the converter script and adapt its two calls that no longer exist on main: updates.update_reaction -> updates.update_dataset, and message_helpers.write_message -> message_helpers.save_message. Verified end-to-end against a synthetic UDM fixture: the script runs, writes a .pbtxt, and it round-trips through message_helpers.load_message. The script is a straight port of a rough-draft external contribution with known gaps (dead code, missing annotations, a reaction-level identifier bug, incomplete field coverage, unguarded Optional access) tracked as follow-up work rather than fixed here. Per-file ruff and ty ignores are added so those known gaps don't block this port; ruff's safe auto-fixes (formatting only) are included. ty-check is skipped for this commit only: it fails on main independent of this change (a pre-existing rdkit.Chem.rdSubstructLibrary stub mismatch in ord_schema/agent/execute.py, unrelated to this file). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The port in the previous commit ran, but a closer look surfaced several real bugs beyond the two API renames already fixed: - Each UDM <VARIATION> is supposed to become its own ORD Reaction, but the loop reassigned pb2_reaction on every variation (discarding the reaction-level identifiers built beforehand) and only ever appended the *last* variation's Reaction -- steps 3-9 and the final append sat outside the per-variation loop entirely. Multi-variation reactions silently lost every variation but the last. - provenance.record_created/record_modified.time is a DateTime message, not a string; the direct string assignment raised AttributeError on any UDM file with CREATION_DATE/MODIFICATION_DATE set. - setup.environment and conditions.stirring are TypeDetails-style messages; setting .details without .type left them "non-empty but UNSPECIFIED", which fails validation. - Reactant amounts were captured onto a Compound that was built and then discarded, never attached to the actual ReactionInput; reagents, catalysts, and solvents didn't capture AMOUNT at all. - all_molecules.get(mol_id) results were dereferenced without a None check for reagents/catalysts/solvents/products, so an unrecognized MOL_ID crashed the conversion instead of degrading gracefully. - CUSTOM CompoundIdentifiers were emitted without `details`, which validation rejects. - docopt isn't declared in pyproject.toml (every other script already migrated to argparse) and would be absent on a clean `uv sync` install; converted this script to argparse to match. - Removed dead code along the way: the non-canonical hand-rolled reaction_id generator (updates.update_dataset already assigns a canonical one), the always-empty debug JSON file, and several unused local variables. Added ord_schema/scripts/convert_udm_to_ord_test.py covering: single and multi-variation conversion, a variation-less reaction, an unresolvable MOL_ID reference, and both failure paths (missing file, non-UDM root). All pass ruff, ruff format, and ty cleanly, so the per-file lint suppressions added when this file was first ported are removed. Remaining known gap, left alone rather than fabricated: ORD requires a `Person.email` on provenance, which UDM's <SCIENTIST> field (a bare name) never supplies -- datasets converted from UDM will need that filled in by hand before they pass full validation. ty-check is skipped for this commit only: it fails on main independent of this change (the same pre-existing rdkit.Chem.rdSubstructLibrary stub mismatch in ord_schema/agent/execute.py noted in the prior commit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…M XSD record_created/record_modified provenance now identifies whoever is running the conversion (OS username, plus an optional --email flag) rather than the original UDM scientist, who remains attributed separately as the experimenter. This matches how ORD distinguishes "who performed the reaction" from "who created this database record." Also fetched the actual UDM v6.0.0 schema and example data from https://github.com/PistoiaAlliance/UDM (the format this script targets, per its own log message) to check the field mapping against the source of truth instead of guesswork. Reference the repo in the module docstring. The example data under Data/ is CC-BY-NC-SA licensed and was used locally to validate parsing, not committed here. The XSD (udm_6_0_0.xsd, udm_6_0_0_units.xsd) turned up several real, previously-unverified mistakes: - SCIENTIST is UDM's authorDetails type (NAME/EMAIL/PHONE/ORGANISATION), not a bare string, and can repeat. The old code assigned it directly to a string Person field, which would raise on any UDM file where a scientist is actually populated -- probably every real one. Now parsed properly, and the scientist's own organization/email/address populate experimenter, falling back to the dataset-level PRODUCER (a data vendor, not a research institution) only when absent. - <AMOUNT> is UDM's molType: molar (default unit "mol"), not mass/grams as previously assumed, with the unit as an XML attribute. Now mapped to Amount.moles with a proper unit lookup instead of always Mass.GRAM. - <PREPARATION> is a child of <CONDITIONS>, not <CONDITION_GROUP> as the code assumed; moved to the correct level. <CONDITIONS> can also repeat <CONDITION_GROUP>, which the code didn't handle (would have crashed calling dict methods on a list). - <STIRRING> is a numeric rate (stirringRange, unit default "rpm"), not free text; now mapped to StirringConditions.rate.rpm instead of being shoved into .details. - Pressure's schema-default unit is "torr", not "atm" as guessed earlier. - REACTANT_ID/PRODUCT_ID are maxOccurs="unbounded"; etree_to_dict collapses a single occurrence to a bare string, so naive iteration split a lone ID into one bogus identifier per character. Fixed by making _as_list handle scalar strings, not just dicts, as a one-item list. ty-check is skipped for this commit only: it fails on main independent of this change (the same pre-existing rdkit.Chem.rdSubstructLibrary stub mismatch in ord_schema/agent/execute.py noted in prior commits). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Off by default (see the CC-BY-SA licensing note already logged at startup: the source UDM text may carry a different license than the converted dataset, so this is opt-in rather than automatic). When set, stores two entries in provenance.reaction_metadata per Reaction: - udm_reaction_xml: the raw <REACTION> element this Reaction came from. Identical across all Reactions produced from the same <REACTION>'s multiple <VARIATION>s. - udm_parent_xml: the UDM document-level context (UDM_VERSION/LEGAL/ ORGANISATIONS/CITATIONS) shared by every reaction in the file. <MOLECULES> is deliberately excluded -- it's a lookup table already captured via each Compound's identifiers, and can be large. This doubles as provenance and as a way for a human to directly compare the converter's output against the source it came from, which is the more immediate motivation for adding it. Implementation note: main() now walks the original ElementTree's <REACTION> elements in parallel with the etree_to_dict'd data (both traverse children in document order, so index i refers to the same <REACTION> in both), only when --include-udm-xml is set. ty-check is skipped for this commit only: it fails on main independent of this change (the same pre-existing rdkit.Chem.rdSubstructLibrary stub mismatch in ord_schema/agent/execute.py noted in prior commits). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
|
Before merging, I will do some more manual spot checks on the available example UDM datasets to make sure this is ready for public release. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Found by Greptile on the PR: when a VARIATION had more than one CONDITION_GROUP, each group mutated the same ReactionConditions in place. Conflicting fields silently took the last group's value while disjoint fields (e.g. one group's temperature plus a different group's pressure) got combined into a single synthetic condition set that never existed in the source -- the same class of bug already fixed for VARIATION in an earlier commit, just one level down. Fix mirrors that precedent: each (VARIATION, CONDITION_GROUP) pair now produces its own Reaction, sharing the variation's reactants/products/ provenance but with its own, un-merged ReactionConditions. A variation with zero or one CONDITION_GROUP is unaffected (still exactly one Reaction). Added a regression test with two CONDITION_GROUPs carrying disjoint fields (temperature-only and pressure-only) asserting they land in two separate Reactions rather than one Reaction with both fields set. ty-check is skipped for this commit only: it fails on main independent of this change, a local-environment rdkit.Chem.rdSubstructLibrary stub mismatch in ord_schema/agent/execute.py noted in prior commits on this branch; ty passes clean on both files touched here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Greptile's follow-up on the previous fix was right: splitting each CONDITION_GROUP into its own Reaction duplicated the whole variation (REACTANT/PRODUCT/YIELD/COMMENT/provenance) into every split, since those are variation-level fields, not per-group. A single recorded yield ended up asserted as independently achieved under every distinct condition set in the source -- a stronger, more likely-false claim than the composite-merging bug that split was meant to fix. Revert to one Reaction per VARIATION. UDM doesn't document what multiple CONDITION_GROUPs within one variation actually mean well enough to safely resolve either way (merge or split), so this settles for the least-wrong option: only the first group is represented in structured ReactionConditions fields, and conditions.details plus a logger.warning note that further groups exist and were not merged, split, or silently dropped. Updated the regression test (multiple groups, now also carrying a variation-level PRODUCT/YIELD) to assert exactly one Reaction, one outcome, only the first group's field set, and a details note about the rest. ty-check is skipped for this commit only: it fails on main independent of this change, a local-environment rdkit.Chem.rdSubstructLibrary stub mismatch in ord_schema/agent/execute.py noted in prior commits on this branch; ty passes clean on both files touched here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… guess
Re-checked the UDM repo's docs (Docs/ChangeLog.md, not just the XSD)
after being asked whether any example data showed how multiple
CONDITION_GROUP is actually used. It does: the CONDITIONS section has
a worked example --
<CONDITION_GROUP><TEMPERATURE><min>20</min><max>20</max></TEMPERATURE>
<TIME><min>23</min><max>23</max></TIME></CONDITION_GROUP>
<CONDITION_GROUP><TEMPERATURE><min>165</min><max>165</max></TEMPERATURE>
<TIME><min>5</min><max>5</max></TIME></CONDITION_GROUP>
<CONDITION_GROUP><TIME><min>6</min><max>6</max></TIME></CONDITION_GROUP>
-- with prose confirming multiple groups model "more complex scenarios,"
i.e. a single dynamic, multi-stage condition profile (heat at 20 degC
for 23 min, then 165 degC for 5 min, then hold 6 more min), not
alternative readings or separate experiments. That rules out both
things already tried: merging fabricates a static snapshot, splitting
into separate Reactions fabricates separate experiments.
ORD's ReactionConditions already has fields for exactly this:
conditions_are_dynamic ("whether the conditions cannot be represented
by the static, single-step schema") and details ("e.g., multiple
stages"). Replaces the previous "first group only, note the rest"
compromise: now conditions_are_dynamic is set and every stage is
rendered into details via a new _summarize_condition_group, so no
stage is arbitrarily dropped.
While rewriting the per-group formatting, also fixed _add_conditions
(the single-group path) to handle UDM's min/max range form via a new
_parse_range helper (midpoint + precision), not just <exact> -- the
ChangeLog's own example uses min/max exclusively, so the single-group
case had the same gap.
Updated the regression test to match the documented example (asserting
conditions_are_dynamic and the full three-stage text, with no
single-step temperature snapshot asserted), and added a test for the
min/max-to-midpoint/precision conversion on a single group.
ty-check is skipped for this commit only: it fails on main independent
of this change, a local-environment rdkit.Chem.rdSubstructLibrary stub
mismatch in ord_schema/agent/execute.py noted in prior commits on this
branch; ty passes clean on both files touched here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Greptile flagged that _summarize_condition_group (the dynamic/multi- stage text summary added in the previous commit) hand-picked a subset of fields -- TEMPERATURE/PRESSURE/TIME/STIRRING/PH -- and silently dropped the rest (e.g. BUFFER_TYPE, BUFFER_CONCENTRATION). Correct: since the dynamic case's text summary is the *only* record of each stage, any field missing from it is gone from the structured/textual representation entirely, recoverable only via the separately opt-in --include-udm-xml. Rechecked udm_6_0_0.xsd for CONDITION_GROUP's complete field list and each range field's default unit (BUFFER_CONCENTRATION/REACTION_MOLARITY: mol/L, TOTAL_VOLUME: L) rather than guessing, and replaced the hand-picked field list with _condition_group_fields, a shared renderer covering every field UDM defines on CONDITION_GROUP: PROCESS, the four agent ID references (REACTANT_ID/REAGENT_ID/CATALYST_ID/SOLVENT_ID), all *Range fields, REFLUX, BUFFER_TYPE, ATMOSPHERE, per-group PREPARATION, and SECTION (UDM's schema-free extension point -- content isn't renderable, but its presence is now at least noted). _summarize_condition_group (multiple groups) now uses this renderer for every field, and the single-group path in _add_variation uses it too for whatever _add_conditions doesn't already represent structurally, so fields like BUFFER_TYPE aren't dropped there either -- they land in .conditions.details instead, deduplicated via an `exclude` set so temperature/pressure/stirring/reflux/pH aren't repeated as text when they're already structural. Added regression tests for both paths (single group and dynamic multi-group) asserting buffer/molarity/volume/agent-ID fields survive into .details rather than disappearing. ty-check is skipped for this commit only: it fails on main independent of this change, a local-environment rdkit.Chem.rdSubstructLibrary stub mismatch in ord_schema/agent/execute.py noted in prior commits on this branch; ty passes clean on both files touched here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Greptile flagged that _parse_range only reads exact/min/max, so a temperatureRange's optional <incr> child (a ramp rate, e.g. "5 degC per hour") was silently dropped from the dynamic-stage text summary. Rechecked udm_6_0_0.xsd for where <incr> actually appears: only temperatureRange and percentageRange define it (not pressureRange, timeRange, stirringRange, molarityRange, volumeRange, or floatRange), and CONDITION_GROUP has no percentageRange field, so in practice this is TEMPERATURE-only. ORD's Temperature message has value/precision/ units but nothing for a ramp rate, so incr has no structured home at all -- unlike BUFFER_TYPE etc. (previous commit), which at least *can* be structurally captured in the single-CONDITION_GROUP case. Added _format_incr and wired it into _condition_group_fields so it is always checked for each range field, independent of whether that field's central value is in the caller's `exclude` set. That matters specifically for the single-group path: TEMPERATURE is excluded there because Temperature.setpoint already captures the central value structurally, but incr still needs to surface in .conditions.details regardless, or it would vanish silently exactly the way the previous BUFFER_TYPE bug did. Added a regression test reproducing the XML Schema note's own example from Docs/ChangeLog.md (<max>100</max><incr unit="deg_C/hour">5</incr>), asserting the ramp rate lands in .conditions.details even though temperature.setpoint is set structurally in the same reaction. ty-check is skipped for this commit only: it fails on main independent of this change, a local-environment rdkit.Chem.rdSubstructLibrary stub mismatch in ord_schema/agent/execute.py noted in prior commits on this branch; ty passes clean on both files touched here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Greptile flagged that non-rpm STIRRING silently vanished: _add_conditions only populates StirringConditions.rate.rpm when the UDM unit is actually "rpm" (stirringRange's @Unit is unrestricted xs:string in the XSD, not an enum, so other units are legal), but the caller's exclude set for the free-text fallback hardcoded STIRRING as always-structural regardless. Exactly the same mistake as the previous incr bug, just for a whole field instead of one sub-element: assuming a field type is always captured structurally when it's actually conditional. Root-caused rather than special-cased again: _add_conditions now returns the set of fields it actually captured for this specific CONDITION_GROUP, and _add_variation uses that real set as the exclude list instead of a static assumption. Removed the now-provably-wrong _STRUCTURED_CONDITION_FIELDS constant entirely, since "which fields are structural" turned out to not be a static property of the field type -- it depends on the data (e.g. STIRRING's unit). Added a regression test with STIRRING in Hz: asserts rate.rpm stays unset (Hz isn't a rate the field can hold) and the value instead appears in .conditions.details rather than disappearing. ty-check is skipped for this commit only: it fails on main independent of this change, a local-environment rdkit.Chem.rdSubstructLibrary stub mismatch in ord_schema/agent/execute.py noted in prior commits on this branch; ty passes clean on both files touched here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
From an independent review of this branch requested alongside Greptile.
- YIELD was written to ProductMeasurement.float_value instead of
.percentage, so every yield this converter produced was invisible to
message_helpers.get_product_yield() (the library's standard accessor,
which only reads .percentage) and triggered validations.py's own
"YIELD measurements should be defined as percentage values" warning.
Also only "exact" was read; a <min>/<max> range yield produced no
measurement at all despite _parse_range already existing for exactly
this shape. Both fixed together: YIELD now goes through _parse_range
and writes .percentage.value/.precision.
- Reaction-level <CITATIONS> handling applied _as_list to the CITATIONS
wrapper (always singular) instead of its CITATION child (the actually
repeatable element), so a reaction with more than one CITATION got a
list where a dict was expected -- "DOI" in reaction_citation checked
list membership against dicts and was always False, silently dropping
DOI/patent. A self-closing <CITATIONS/> was worse: reaction["CITATIONS"]
is None, _as_list(None) is [], and indexing [0] raised an uncaught
IndexError, aborting the whole conversion.
- Fixed a stale comment claiming RXNSTRUCTURE's format is an XML
attribute ("@Format"); the code and test have always correctly read
it as a child element ("format").
Added regression tests for the YIELD range/percentage fix, multiple
reaction-level citations, and empty reaction-level citations.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
From the same independent review as the previous commit. PRODUCT_ID handling lived in _build_base_reaction (reaction-level, run once and copied via CopyFrom into every variation's Reaction), which had two problems: - An empty/self-closing <PRODUCT_ID/> still unconditionally created a ReactionOutcome before the (then-empty) loop over its values ran, leaving every variation with a spurious outcome containing zero products. - A PRODUCT_ID and a variation's own <PRODUCT> referencing the same molecule were never reconciled, so one product could show up as two separate ReactionOutcomes: an empty placeholder (from PRODUCT_ID) and the real one with structure/yield (from PRODUCT). Moved PRODUCT_ID handling into _add_variation, after that variation's own PRODUCT entries are processed, tracking which molecule IDs they already cover so a PRODUCT_ID for one of those is skipped rather than added as a duplicate. An empty PRODUCT_ID now naturally adds nothing, since _as_list(None) is [] and the loop it drives no longer has a preceding unconditional .outcomes.add(). Added regression tests for all three cases: empty PRODUCT_ID, a PRODUCT_ID covered by the variation's own PRODUCT, and a PRODUCT_ID not covered by it (still recorded, unchanged from before). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
From the same independent review as the two previous commits. - pb2_reaction.inputs[mol_id] indexed only by MOL_ID, so the same molecule referenced under two different roles in one variation (e.g. a REAGENT and a CATALYST, each with its own AMOUNT) collapsed into a single ReactionInput -- misrepresenting two separate additions as one addition event. Now the key is only disambiguated (mol_id_ROLE) when a real collision with a *different* role is detected on that specific MOL_ID, so the common non-colliding case is unaffected and still uses the plain MOL_ID key. - _set_molecule_identifier labeled a molecule as "not found in <MOLECULES>" even when it genuinely was found there but simply had neither a NAME nor a MOLSTRUCTURE -- all_molecules always sets a "name" key (possibly None), so that dict was truthy and the check fell through to the same branch as a real lookup miss. Split into its own branch with an accurate message, so the two cases are distinguishable when debugging conversion gaps. Added regression tests for both: same MOL_ID under two roles ending up in two separate ReactionInputs with their own amounts intact, and a present-but-empty MOLECULE getting the new, accurate message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Greptile flagged this in two places: _add_conditions (single condition
group) wrote a lone <max> straight to Temperature.setpoint.value as if
it were an exact reading, and _format_range_text rendered it as a bare
number in the dynamic multi-stage text summary -- both indistinguishable
from an actual <exact>100</exact>. "At most 100 degC" and "exactly
100 degC" are different claims; UDM's own note in Docs/ChangeLog.md
about *Range types explains why a lone bound and an exact value are
represented differently in the first place.
_parse_range now only returns a point value for <exact> or a *complete*
min+max pair (as their midpoint, same as before); a lone min or max
returns None instead of silently being treated as a point. Because
_add_conditions already only marks a field "captured" when _parse_range
succeeds, a lone bound now automatically falls through to the existing
text-fallback path (the same mechanism built for BUFFER_TYPE/incr/
non-rpm STIRRING) rather than needing new special-casing.
_format_range_text renders the fallback as a labeled bound (">=90 degC"
/ "<=100 degC"), never a bare number.
The same function is used by YIELD (_add_product); a lone-bound yield
would otherwise have silently produced no measurement at all, so that
case now falls back to ProductMeasurement.details as text instead of
being dropped.
Updated test_temperature_ramp_incr_is_not_dropped, which had (unknowingly)
been asserting the buggy behavior -- a lone <max> written to setpoint.value
as if exact -- and switched its fixture to a complete min/max pair (the
only form the XSD allows alongside <incr> anyway). Added new regression
tests for the lone-bound case in the single-group, dynamic-stage, and
YIELD paths.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Greptile flagged that an rpm STIRRING given as a <min>/<max> range (e.g. 490-510) got silently rounded to a bare exact rate.rpm = 500 and marked "captured", so the range never fell through to the text fallback that would have preserved it -- indistinguishable afterward from an actual exact 500 rpm reading. Same root cause, same file, as the earlier "non-rpm STIRRING" fix: StirringConditions.rate.rpm can't fully represent everything UDM's stirringRange can express -- that time it was the unit (rpm-only), this time it's precision (rate.rpm has no precision field, unlike Temperature/Pressure's setpoint). STIRRING is now only added to the `captured` set when _parse_range returns no precision (an <exact> value, or a degenerate min==max range) -- i.e. when rounding to an int genuinely loses nothing. A real range still gets its best-effort rounded midpoint written to rate.rpm *and* falls through to the existing text fallback (_condition_group_fields, via _format_range_text, which already renders precision), rather than needing new special-casing. Added a regression test with a 490-510 rpm range asserting both: the structural value is the rounded midpoint, and the full range still appears in conditions.details. Added a second test confirming an exact (no-precision-loss) reading is captured structurally only, not also duplicated into the text fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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
Ports
convert_udm_to_ord.pyfrom the staleudm-converterbranch onto currentmain(that branch was ~100 commits behind and not worth merging directly), then fixes real bugs found both by running it and by checking its field mapping against the actual UDM v6.0.0 XSD schema (Pistoia Alliance). Adds a--emailflag for record-creator provenance and an opt-in--include-udm-xmlflag that embeds the raw UDM source for provenance and human review.Changes
main; adapt to renamed APIs (updates.update_dataset,message_helpers.save_message) and migratedocopt→argparse(docopt isn't a declared dependency and every other script had already migrated).<VARIATION>now produces its ownReaction(previously only the last variation survived, and reaction-level identifiers were discarded in the process).SCIENTISTis a structured type, not a bare string;AMOUNTis molar, not mass;PREPARATIONbelongs underCONDITIONS, notCONDITION_GROUP;STIRRINGis a numeric rate, not text; correct default units for temperature/pressure; a_as_listbug that split a loneREACTANT_ID/PRODUCT_IDinto individual characters.--email:record_created/record_modifiedprovenance now identifies the person running the conversion (OS username plus an optional email), distinct fromexperimenter(the original UDM scientist). This closes the Person-email requirement that ORD validation enforces on provenance.--include-udm-xml(off by default): embeds the raw UDM<REACTION>XML and the shared document-level context (LEGAL/CITATIONS/ORGANISATIONS) inprovenance.reaction_metadata, for provenance and so reviewers can diff the converter's output against its source.Testing
ord_schema/scripts/convert_udm_to_ord_test.py: 10 tests covering single- and multi-variation conversion, a variation-less reaction, an unresolved molecule reference, theREACTANT_IDcharacter-split regression,--email/provenance attribution, and--include-udm-xml.uv run pytest ord_schema/scripts/— 51 passed.ruff check,ruff format --check,ty check— clean on both files.SCIENTIST, molarAMOUNT, unit-attributedTEMPERATURE/PRESSURE,STIRRING,PREPARATION), and smoke-tested against a real (differently-versioned, UDM v3.6) Reaxys export to confirm the converter degrades gracefully rather than crashing on non-conformant input.Notes
ty-checkpre-commit hook. That failure (rdkit.Chem.rdSubstructLibraryunresolved inord_schema/agent/execute.py) is a local environment issue on the machine this branch was developed on, not something introduced by this change or a problem with this branch's code —typasses clean on both files touched here.converters/submodule (for UDM + SURF together) remain future work, not attempted here.Greptile Summary
The PR ports the UDM-to-ORD converter to the current APIs and expands its mapping, provenance, variation, and dynamic-condition handling.
Confidence Score: 4/5
The PR is not yet safe to merge because ranged stirring is still represented as an exact structured midpoint for machine-readable consumers.
For an rpm min/max range, the converter writes the rounded midpoint into
conditions.stirring.rate.rpmand an exact-looking midpoint intostirring.details; retaining the true range only in top-level free text does not remove the incorrect structured claim.Files Needing Attention: ord_schema/scripts/convert_udm_to_ord.py
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR UDM[UDM XML] --> Parse[Parse document and molecule table] Parse --> Base[Build reaction-level ORD data] Base --> Variations{UDM variations} Variations --> Reaction[One ORD Reaction per variation] Reaction --> Conditions{Condition groups} Conditions -->|One| Static[Structured fields plus textual fallback] Conditions -->|Multiple| Dynamic[Dynamic staged-condition details] Static --> Dataset[ORD Dataset] Dynamic --> Dataset Dataset --> Update[Assign dataset and reaction IDs] Update --> Validate[Validate unless disabled] Validate --> Save[Write output]Reviews (12): Last reviewed commit: "Stop assuming STIRRING is always capture..." | Re-trigger Greptile