From 3a91f0bf60c720fa07889967e44850005922b0d7 Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 14:34:37 +0100 Subject: [PATCH 01/15] Port convert_udm_to_ord.py onto current main 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 --- ord_schema/scripts/convert_udm_to_ord.py | 648 +++++++++++++++++++++++ pyproject.toml | 11 +- 2 files changed, 658 insertions(+), 1 deletion(-) create mode 100644 ord_schema/scripts/convert_udm_to_ord.py diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py new file mode 100644 index 00000000..670e9fc1 --- /dev/null +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -0,0 +1,648 @@ +# Copyright 2025 Open Reaction Database Project Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Builds a Dataset from a set of (UDM) Reactions. + +Given a UDM file, convert to .pbtxt for ORD + +Usage: + convert_udm_to_ord.py --input= [--output=] [--name=] [--description=] [--no-validate] + +Options: + --input= XML filename in UDM format + --output= Output Dataset filename (*.pbtxt) + --name= Name for this dataset + --description= Description for this dataset + --no-validate If set, do not run validations on reactions +""" +# import glob + +import json +import os.path +import random +import xml.etree.ElementTree as ET +from collections import defaultdict + +import docopt + +from ord_schema import message_helpers, updates, validations +from ord_schema.logging import get_logger +from ord_schema.proto import dataset_pb2, reaction_pb2 + +logger = get_logger(__name__) + + +def main(kwargs): + logger.info("Starting conversion from UDM v6.0.0 to ORD.") + logger.info( + "*** Important message - The Open Reaction Database repository, ord-data, uses the CC-BY-SA license" + " for all data. " + "Please do not push your converted dataset to ord-data unless you have the authority to " + "change the license on the data to CC-BY-SA. ***" + ) + + # Default input and output file names + inputfile = "udm_dataset.xml" + outputfile = "ord_dataset.pbtxt" + + # Pull input filename from argument parameters + if kwargs["--input"]: + inputfile = kwargs["--input"] + + # If the input file is not found, exit with an error message + if not os.path.isfile(inputfile): + logger.info( + "Error - Conversion failed. Please check that the --input file exists at the specified location." + ) + exit(1) + + # Load UDM file and parse XML into a dictionary + udm_tree = ET.parse(inputfile) + root = udm_tree.getroot() + # for child in root: + # print(child.tag, child.attrib) + udm_reactions = etree_to_dict(root) + + # is expected to be the root element of the UDM file - if it isn't, we exit with an error. + if "UDM" not in udm_reactions: + logger.info( + "Error - Input file is not UDM format - please ensure the tag is used as the root of your " + ".xml file." + ) + exit(1) + + # Access the data in the .xml file. + udm_reactions = udm_reactions["UDM"] + + # Determine dataset name and description - if given, the will be used as the name. + # If a global DOI is given, this will be included in the dataset description. + dataset_name = "" + dataset_description = "" + if "LEGAL" in udm_reactions: + if "TITLE" in udm_reactions["LEGAL"]: + outputfile = udm_reactions["LEGAL"]["TITLE"] + ".pbtxt" + dataset_name = udm_reactions["LEGAL"]["TITLE"] + if "DOI" in udm_reactions["LEGAL"]: + dataset_description = "UDM dataset DOI: " + udm_reactions["LEGAL"]["DOI"] + + # Set the dataset name if explicitly provided (overrides what's in the UDM file) + if kwargs["--name"]: + dataset_name = kwargs["--name"] + + # Set the dataset description if explicitly provided (overrides what's in the UDM file) + if kwargs["--description"]: + dataset_description = kwargs["--description"] + + # Set the output file name if explicitly provided (overrides what's in the UDM file) + if kwargs["--output"]: + outputfile = kwargs["--output"] + + # Inspect UDM reactions + # print(str(udm_reactions)) + + # List of ORD reactions-to-be + ord_reactions = [] + pb2_reactions = [] + + # This is unlikely to be hit, but just in case + if "REACTIONS" not in udm_reactions: + logger.info("Error - <REACTIONS> element not found in input .xml file.") + exit(1) + + # If there is just one reaction in the entire file, we have to change our approach. + # Create a list of every reaction in the file. + udm_reactions_list = [] + if isinstance(udm_reactions["REACTIONS"]["REACTION"], dict): + udm_reactions_list.append(udm_reactions["REACTIONS"]["REACTION"]) + else: + for reaction in udm_reactions["REACTIONS"]["REACTION"]: + udm_reactions_list.append(reaction) + + all_molecules = {} + for molecule in udm_reactions["MOLECULES"]["MOLECULE"]: + mol_data = dict() + mol_data["name"] = molecule["NAME"] + if "MOLSTRUCTURE" in molecule: + mol_data["molblock"] = molecule["MOLSTRUCTURE"] + all_molecules[molecule["@ID"]] = mol_data + + # Loop over each UDM reaction. UDM reactions may have one or more Variations. + # We will treat each Variation as a separate ORD Reaction. + # May be 1 or multiple, with null checks + # Return errors if UDM format is wrong + # Return warnings if data has to be omitted if not supported in ORD + for reaction in udm_reactions_list: + # Create a blank dictionary for each reaction. + pb2_reaction = reaction_pb2.Reaction() + + # Step 1 of 9: Identifiers + # Used to identify molecules + + pb2_inputs = [] + + if "REACTANT_ID" in reaction: + molinput = pb2_reaction.inputs[""] + reactant_list = reaction["REACTANT_ID"] + for reactant_id in reactant_list: + molcomponent = molinput.components.add() + molcomponent.identifiers.add( + type="CUSTOM", details="REACTANT_ID from UDM" + ) + molcomponent.identifiers[0].value = reactant_id + + # May be multiple RXNSTRUCTs + if "RXNSTRUCTURE" in reaction: + rxnstructures = [] + if isinstance(reaction["RXNSTRUCTURE"], dict): + rxnstructures.append(reaction["RXNSTRUCTURE"]) + else: + for structure in reaction["RXNSTRUCTURE"]: + rxnstructures.append(structure) + + for structure in rxnstructures: + udmformat = 1 + if "format" in structure: + udmformat = structure["format"] + + # Default reaction identifier type will be unspecified. + ordtype = 0 + orddetails = "" + + # For types not supported in ORD, the type will be CUSTOM + # and the details will contain the name of the type. + if udmformat == "cdxml": + ordtype = 1 + orddetails = "cdxml" + elif udmformat == "rinchi": + ordtype = 5 + elif udmformat == "rsmiles": + ordtype = 2 + elif udmformat == "rxn": + ordtype = 1 + orddetails = "rxn" + + udmvalue = None + if "value" in structure: + udmvalue = structure["value"] + + pb2_reaction.identifiers.add( + type=ordtype, details=orddetails, value=udmvalue + ) + + # Step 2 of 9: Inputs + # Used for reaction inputs, i.e. reactants, catalysts, solvents etc. + + # Reactions in UDM are more VARIATIONS on a reaction conducted during EXPERIMENTS + # Roles for Compounds and Products together in one enum for ORD. + # UNSPECIFIED, REACTANT, REAGENT, SOLVENT, CATALYST, WORKUP, INTERNAL_STANDARD, AUTHENTIC_STANDARD + # PRODUCT, BYPRODUCT, SIDE_PRODUCT + # Of these, REACTANT, REAGENT, SOLVENT, CATALYST, and PRODUCT are represented in UDM as separate data structures. + # They allow MULTIPLE of each one in each reaction with MULTIPLE compounds. + ord_inputs = [] + + # If there is one variation, it is a dict + variations = [] + if "VARIATION" in reaction: + if isinstance(reaction["VARIATION"], dict): + variations.append(reaction["VARIATION"]) + else: + for variation in reaction["VARIATION"]: + variations.append(variation) + + # If there are multiple variations, each will be its own reaction. + for variation in variations: + pb2_reaction = reaction_pb2.Reaction() + + reactants = [] + if "REACTANT" in variation: + if isinstance(variation["REACTANT"], dict): + reactants.append(variation["REACTANT"]) + else: + for reactant in variation["REACTANT"]: + reactants.append(reactant) + + reagents = [] + if "REAGENT" in variation: + if isinstance(variation["REAGENT"], dict): + reagents.append(variation["REAGENT"]) + else: + for reagent in variation["REAGENT"]: + reagents.append(reagent) + + catalysts = [] + if "CATALYST" in variation: + if isinstance(variation["CATALYST"], dict): + catalysts.append(variation["CATALYST"]) + else: + for catalyst in variation["CATALYST"]: + catalysts.append(catalyst) + + solvents = [] + if "SOLVENT" in variation: + if isinstance(variation["SOLVENT"], dict): + solvents.append(variation["SOLVENT"]) + else: + for solvent in variation["SOLVENT"]: + solvents.append(solvent) + + for reactant in reactants: + pb2_compound = reaction_pb2.Compound() + compound = dict() + # TODO: Check the following translation from MOLECULE to identifiers. + compound["identifiers"] = reactant["MOLECULE"] + # compound["amount"] = reactant["AMOUNT"] + compound["reaction_role"] = [] + compound["is_limiting"] = [] + compound["preparations"] = [] + compound["source"] = [] + compound["features"] = [] + compound["analyses"] = [] + compound["texture"] = [] + + if "MOLECULE" in reactant: + molval = all_molecules.get(reactant["MOLECULE"]["@MOL_ID"]) + # pb2_reaction.identifiers.add(type=0, details='', value=molval) + molinput = pb2_reaction.inputs[reactant["MOLECULE"]["@MOL_ID"]] + molcomponent = molinput.components.add() + if "molblock" in molval: + molcomponent.identifiers.add( + type="MOLBLOCK", details="MOLECULE -> MOLSTRUCTURE from UDM" + ) + molcomponent.identifiers[0].value = molval["molblock"] + else: + molcomponent.identifiers.add(type="CUSTOM") + if molval is not None: + molcomponent.identifiers[0].value = molval["name"] + if "MOLECULE" in reactant and "NAME" in reactant["MOLECULE"]: + pb2_compound.identifiers.add(value=reactant["MOLECULE"]["NAME"]) + if "AMOUNT" in reactant: + convertedAmount = float(reactant["AMOUNT"]) + pb2_compound.amount.mass.value = convertedAmount + + pb2_components = [] + pb2_components.append(pb2_compound) + + pb2_input = reaction_pb2.ReactionInput() + pb2_input.components.append(pb2_compound) + pb2_inputs.append(pb2_input) + + # pb2_reaction.inputs.update(key=pb2_input, value=pb2_input) + # pb2_input.components.add(pb2_compound) + # pb2_reaction.inputs.update(key=reactant["MOLECULE"]["@MOL_ID"], value=pb2_input) + # updates.update_reaction(pb2_reaction) + + # Step 4 of 9: Conditions + # Describes the reaction conditions. + + for reagent in reagents: + if "MOLECULE" in reagent: + molval = all_molecules.get(reagent["MOLECULE"]["@MOL_ID"]) + # print(molval) + # pb2_reaction.identifiers.add(type=0, details='', value=molval) + molinput = pb2_reaction.inputs[reagent["MOLECULE"]["@MOL_ID"]] + molcomponent = molinput.components.add() + if "molblock" in molval: + molcomponent.identifiers.add( + type="MOLBLOCK", details="MOLECULE -> MOLSTRUCTURE from UDM" + ) + molcomponent.identifiers[0].value = molval["molblock"] + else: + molcomponent.identifiers.add(type="CUSTOM") + if molval["name"] is not None: + molcomponent.identifiers[0].value = molval["name"] + molcomponent.reaction_role = reaction_pb2.ReactionRole.REAGENT + + for catalyst in catalysts: + if "MOLECULE" in catalyst: + molval = all_molecules.get(catalyst["MOLECULE"]["@MOL_ID"]) + # print(molval) + # pb2_reaction.identifiers.add(type=0, details='', value=molval) + molinput = pb2_reaction.inputs[catalyst["MOLECULE"]["@MOL_ID"]] + molcomponent = molinput.components.add() + if "molblock" in molval: + molcomponent.identifiers.add( + type="MOLBLOCK", details="MOLECULE -> MOLSTRUCTURE from UDM" + ) + molcomponent.identifiers[0].value = molval["molblock"] + else: + molcomponent.identifiers.add(type="CUSTOM") + if molval["name"] is not None: + molcomponent.identifiers[0].value = molval["name"] + molcomponent.reaction_role = reaction_pb2.ReactionRole.CATALYST + + for solvent in solvents: + if "MOLECULE" in solvent: + molval = all_molecules.get(solvent["MOLECULE"]["@MOL_ID"]) + # print(molval) + # pb2_reaction.identifiers.add(type=0, details='', value=molval) + molinput = pb2_reaction.inputs[solvent["MOLECULE"]["@MOL_ID"]] + molcomponent = molinput.components.add() + if "molblock" in molval: + molcomponent.identifiers.add( + type="MOLBLOCK", details="MOLECULE -> MOLSTRUCTURE from UDM" + ) + molcomponent.identifiers[0].value = molval["molblock"] + else: + molcomponent.identifiers.add(type="CUSTOM") + if molval["name"] is not None: + molcomponent.identifiers[0].value = molval["name"] + molcomponent.reaction_role = reaction_pb2.ReactionRole.SOLVENT + + if ( + "CONDITIONS" in variation + and "CONDITION_GROUP" in variation["CONDITIONS"] + ): + pb2_conditions = reaction_pb2.ReactionConditions() + if "TEMPERATURE" in variation["CONDITIONS"]["CONDITION_GROUP"]: + if ( + "exact" + in variation["CONDITIONS"]["CONDITION_GROUP"]["TEMPERATURE"] + ): + pb2_reaction.conditions.temperature.setpoint.value = float( + variation["CONDITIONS"]["CONDITION_GROUP"]["TEMPERATURE"][ + "exact" + ] + ) + if "PRESSURE" in variation["CONDITIONS"]["CONDITION_GROUP"]: + if ( + "exact" + in variation["CONDITIONS"]["CONDITION_GROUP"]["PRESSURE"] + ): + pb2_reaction.conditions.pressure.setpoint.value = float( + variation["CONDITIONS"]["CONDITION_GROUP"]["PRESSURE"][ + "exact" + ] + ) + if "STIRRING" in variation["CONDITIONS"]["CONDITION_GROUP"]: + pb2_reaction.conditions.stirring.details = variation["CONDITIONS"][ + "CONDITION_GROUP" + ]["STIRRING"] + if "REFLUX" in variation["CONDITIONS"]["CONDITION_GROUP"]: + pb2_reaction.conditions.reflux = variation["CONDITIONS"][ + "CONDITION_GROUP" + ]["REFLUX"] + if "PH" in variation["CONDITIONS"]["CONDITION_GROUP"]: + if "exact" in variation["CONDITIONS"]["CONDITION_GROUP"]["PH"]: + pb2_reaction.conditions.ph = float( + variation["CONDITIONS"]["CONDITION_GROUP"]["PH"]["exact"] + ) + + pb2_setup = reaction_pb2.ReactionSetup() + if "PREPARATION" in variation["CONDITIONS"]["CONDITION_GROUP"]: + pb2_reaction.setup.environment = variation["CONDITIONS"][ + "CONDITION_GROUP" + ]["PREPARATION"] + + # pb2_reaction.conditions = pb2_conditions + + # Step 3 of 9: Setup + # Describes the reaction setup. + + pb2_setup = reaction_pb2.ReactionSetup() + # pb2_reaction.setup = pb2_setup + + # Step 5 of 9: Notes + # Extra notes about the reaction. + + pb2_notes = reaction_pb2.ReactionNotes() + # pb2_notes.safety_notes + # pb2_notes.procedure_details + pb2_reaction.notes.procedure_details = "" + + # Step 6 of 9: Observations + # Notes about observations during the reaction. + + pb2_observations = reaction_pb2.ReactionObservation() + # pb2_observations.time + if "VARIATION" in reaction and "COMMENT" in reaction["VARIATION"]: + pb2_observations.comment = reaction["VARIATION"]["COMMENT"] + pb2_reaction.observations.append(pb2_observations) + + # Step 7 of 9: Workups + # Reaction workups + + pb2_workups = reaction_pb2.ReactionWorkup() + # type, details, duration, input, amount, temperature, stirring, target_ph + pb2_reaction.workups.append(pb2_workups) + + # Step 8 of 9: Outcomes + # The outcomes of the reaction - time taken, the products etc. + + pb2_outcomes = reaction_pb2.ReactionOutcome() + # reaction_time, conversion, analyses + + products = [] + + if "PRODUCT_ID" in reaction: + molinput = pb2_reaction.inputs[""] + product_list = reaction["PRODUCT_ID"] + outcome = pb2_reaction.outcomes.add() + for product_id in product_list: + molcomponent = outcome.products.add() + molcomponent.identifiers.add( + type="CUSTOM", details="PRODUCT_ID from UDM" + ) + molcomponent.identifiers[0].value = product_id + + if "VARIATION" in reaction and "PRODUCT" in reaction["VARIATION"]: + if isinstance(reaction["VARIATION"]["PRODUCT"], dict): + products.append(reaction["VARIATION"]["PRODUCT"]) + else: + for product in reaction["VARIATION"]["PRODUCT"]: + products.append(product) + + for udm_product in products: + molecule = udm_product["MOLECULE"] + outcome = pb2_reaction.outcomes.add() + product = outcome.products.add() + if all_molecules.get(molecule["@MOL_ID"]) is not None: + molval = all_molecules.get(molecule["@MOL_ID"]) + if "molblock" in molval: + product.identifiers.add( + type="MOLBLOCK", details="MOLECULE -> MOLSTRUCTURE from UDM" + ) + product.identifiers[0].value = molval["molblock"] + else: + product.identifiers.add(type="CUSTOM") + if molval["name"] is not None: + product.identifiers[0].value = molval["name"] + if "YIELD" in udm_product: + product_measurement = product.measurements.add() + product_measurement.type = ( + reaction_pb2.ProductMeasurement.ProductMeasurementType.YIELD + ) + product_measurement.float_value.value = float( + udm_product["YIELD"]["exact"] + ) + + # Step 9 of 9: Provenance + # Publication and patent details, attribution, other metadata + + ord_provenance = dict() + pb2_provenance = reaction_pb2.ReactionProvenance() + ord_provenance_experimenter = dict() + if "LEGAL" in udm_reactions: + pb2_reaction.provenance.experimenter.organization = udm_reactions["LEGAL"][ + "PRODUCER" + ] + if "VARIATION" in reaction and "SCIENTIST" in reaction["VARIATION"]: + pb2_reaction.provenance.experimenter.name = reaction["VARIATION"][ + "SCIENTIST" + ] + if "LEGAL" in udm_reactions: + pb2_reaction.provenance.record_created.person.organization = udm_reactions[ + "LEGAL" + ]["PRODUCER"] + if "VARIATION" in reaction and "SCIENTIST" in reaction["VARIATION"]: + pb2_reaction.provenance.record_created.person.name = reaction["VARIATION"][ + "SCIENTIST" + ] + if "ORGANISATIONS" in reaction: + pb2_reaction.provenance.city = reaction["ORGANISATIONS"][0]["ORGANISATION"][ + "ADDRESS" + ] + + # experiment start + + if ( + "LEGAL" in udm_reactions + and "DOI" in udm_reactions["LEGAL"] + and "CITATIONS" not in reaction + ): + pb2_reaction.provenance.doi = udm_reactions["LEGAL"]["DOI"] + elif ( + "CITATIONS" in udm_reactions + and "VARIATION" in reaction + and "CITATION" in reaction["VARIATION"] + ): + variation_citation = reaction["VARIATION"]["CITATION"] + variation_doi = "" + for citation in udm_reactions["CITATIONS"]["CITATION"]: + if isinstance(variation_citation, list): + variation_citation = variation_citation[0] + if ( + citation["@ID"] == variation_citation["@CIT_ID"] + and "DOI" in citation + ): + variation_doi = citation["DOI"] + if variation_doi != "": + pb2_reaction.provenance.doi = variation_doi + + if "CITATIONS" in reaction: + pb2_reaction.provenance.doi = reaction["CITATIONS"][0]["CITATION"]["DOI"] + pb2_reaction.provenance.patent = reaction["CITATIONS"][0]["CITATION"][ + "PATENT_NUMBER" + ] + + # publication url is not provided + + if "VARIATION" in reaction and "CREATION_DATE" in reaction["VARIATION"]: + pb2_reaction.provenance.record_created.time = reaction["VARIATION"][ + "CREATION_DATE" + ] + + if "VARIATION" in reaction and "MODIFICATION_DATE" in reaction["VARIATION"]: + pb2_reaction.provenance.record_modified.time = reaction["VARIATION"][ + "MODIFICATION_DATE" + ] + + # ord_provenance["reaction_metadata"] = "" + pb2_reaction.provenance.is_mined = False + + # Generate REACTION ID + pb2_reaction.reaction_id = generate_reaction_id() + + pb2_reactions.append(pb2_reaction) + + # Create the ORD dataset + dataset = dataset_pb2.Dataset( + name=dataset_name, description=dataset_description, reactions=pb2_reactions + ) + + # Assign canonical dataset_id/reaction_ids and provenance updates. + updates.update_dataset(dataset) + + # Validate the dataset + if not kwargs["--no-validate"]: + validations.validate_datasets({"_COMBINED": dataset}) + + # Create the .pbtxt file + message_helpers.save_message(dataset, outputfile) + + # Write JSON file for debug + debugfile = "ORD_UDM_CONVERSION_" + inputfile + ".debug.json" + f = open(debugfile, "w") + f.write(json.dumps({}, indent=4, separators=(", ", ": "))) + f.close() + + # Successfully converted UDM .xml file into ORD dataset! + logger.info( + "Conversion completed successfully! Check ORD_UDM_CONVERSION_" + + inputfile + + ".debug.json file for errors." + ) + + exit(0) + + +# Converts XML tree into Python dictionary. +# https://stackoverflow.com/questions/7684333/converting-xml-to-dictionary-using-elementtree +def etree_to_dict(t): + d = {t.tag: {} if t.attrib else None} + children = list(t) + if children: + dd = defaultdict(list) + for dc in map(etree_to_dict, children): + for k, v in dc.items(): + dd[k].append(v) + d = {t.tag: {k: v[0] if len(v) == 1 else v for k, v in dd.items()}} + if t.attrib: + d[t.tag].update(("@" + k, v) for k, v in t.attrib.items()) + if t.text: + text = t.text.strip() + if children or t.attrib: + if text: + d[t.tag]["#text"] = text + else: + d[t.tag] = text + return d + + +def generate_reaction_id(): + random_id_parts = [ + "0", + "1", + "2", + "3", + "4", + "5", + "6", + "7", + "8", + "9", + "a", + "b", + "c", + "d", + "e", + "f", + ] + new_reaction_id = "ord-" + for i in range(0, 31): + new_reaction_id += random.choice(random_id_parts) + return new_reaction_id + + +# Shows the Usage information if parameters not provided (self documenting) +if __name__ == "__main__": + main(docopt.docopt(__doc__)) diff --git a/pyproject.toml b/pyproject.toml index 2363e5fa..f77d599e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -221,6 +221,15 @@ convention = "google" "**/conftest.py" = ["D"] # Tutorial notebooks legitimately show commented-out alternative code. "**/*.ipynb" = ["ERA001", "S310", "D"] # tutorials download from known URLs; D: cells are narrative, not modules +# convert_udm_to_ord.py is a straight port of a rough-draft external contribution; +# it is functional but has known gaps (dead code, missing annotations/docstrings, +# unvetted XML/random usage) tracked as follow-up work rather than fixed here. +"**/scripts/convert_udm_to_ord.py" = [ + "ANN", "D103", "B007", "C408", "E501", "ERA001", "F841", "G003", + "N806", "PERF402", "PLR1722", "PTH113", "PTH123", "S311", "S314", + "SIM115", "TD002", +] [tool.ty.src] -exclude = ["**/*_pb2.py*"] +# convert_udm_to_ord.py: see the ruff per-file-ignore above for why. +exclude = ["**/*_pb2.py*", "**/scripts/convert_udm_to_ord.py"] From c6099eb73b4c5bcd9c7fe2ca9b15b345eb460e49 Mon Sep 17 00:00:00 2001 From: Ben Deadman <ben.deadman@gmail.com> Date: Fri, 14 Aug 2026 14:57:53 +0100 Subject: [PATCH 02/15] Fix convert_udm_to_ord.py's structural bugs and add test coverage 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> --- ord_schema/scripts/convert_udm_to_ord.py | 916 +++++++----------- ord_schema/scripts/convert_udm_to_ord_test.py | 208 ++++ pyproject.toml | 11 +- 3 files changed, 553 insertions(+), 582 deletions(-) create mode 100644 ord_schema/scripts/convert_udm_to_ord_test.py diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 670e9fc1..a1ea7819 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -13,27 +13,17 @@ # limitations under the License. """Builds a Dataset from a set of (UDM) Reactions. -Given a UDM file, convert to .pbtxt for ORD - -Usage: - convert_udm_to_ord.py --input=<str> [--output=<str>] [--name=<str>] [--description=<str>] [--no-validate] - -Options: - --input=<str> XML filename in UDM format - --output=<str> Output Dataset filename (*.pbtxt) - --name=<str> Name for this dataset - --description=<str> Description for this dataset - --no-validate If set, do not run validations on reactions +Given a UDM file, converts it to a Dataset .pbtxt for ORD. A UDM <REACTION> +may contain multiple <VARIATION> elements; each variation is emitted as its +own ORD Reaction, since UDM variations represent distinct experiments run +under the same reaction identifiers. """ -# import glob -import json -import os.path -import random +import argparse import xml.etree.ElementTree as ET from collections import defaultdict - -import docopt +from pathlib import Path +from typing import Any from ord_schema import message_helpers, updates, validations from ord_schema.logging import get_logger @@ -41,564 +31,372 @@ logger = get_logger(__name__) +UdmDict = dict[str, Any] + +_MOLBLOCK_DETAILS = "MOLECULE -> MOLSTRUCTURE from UDM" + +# UDM roles that map directly onto an ORD ReactionRole. +_ROLES = { + "REACTANT": reaction_pb2.ReactionRole.REACTANT, + "REAGENT": reaction_pb2.ReactionRole.REAGENT, + "CATALYST": reaction_pb2.ReactionRole.CATALYST, + "SOLVENT": reaction_pb2.ReactionRole.SOLVENT, +} + +# UDM RXNSTRUCTURE @format values that map onto an ORD ReactionIdentifierType. +# Anything not listed here (including UDM's default, unlabeled format) falls +# back to UNSPECIFIED; that reaction identifier is then dropped rather than +# emitted with an invalid type (see _build_base_reaction). +_RXNSTRUCTURE_FORMATS: dict[str, tuple[int, str]] = { + "cdxml": (reaction_pb2.ReactionIdentifier.CUSTOM, "cdxml"), + "rinchi": (reaction_pb2.ReactionIdentifier.RINCHI, ""), + "rsmiles": (reaction_pb2.ReactionIdentifier.REACTION_SMILES, ""), + "rxn": (reaction_pb2.ReactionIdentifier.CUSTOM, "rxn"), +} + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parses command-line arguments.""" + parser = argparse.ArgumentParser(description="Convert a UDM dataset to ORD") + parser.add_argument("--input", required=True, help="XML filename in UDM format") + parser.add_argument("--output", help="Output Dataset filename (*.pbtxt)") + parser.add_argument("--name", help="Name for this dataset") + parser.add_argument("--description", help="Description for this dataset") + parser.add_argument( + "--no-validate", + action="store_true", + help="If set, do not run validations on reactions", + ) + return parser.parse_args(argv) + -def main(kwargs): +def main(args: argparse.Namespace) -> None: + """Converts a UDM XML dataset to an ORD Dataset and writes it to disk.""" logger.info("Starting conversion from UDM v6.0.0 to ORD.") logger.info( - "*** Important message - The Open Reaction Database repository, ord-data, uses the CC-BY-SA license" - " for all data. " - "Please do not push your converted dataset to ord-data unless you have the authority to " - "change the license on the data to CC-BY-SA. ***" + "*** Important message - The Open Reaction Database repository, ord-data, " + "uses the CC-BY-SA license for all data. Please do not push your converted " + "dataset to ord-data unless you have the authority to change the license " + "on the data to CC-BY-SA. ***" ) - # Default input and output file names - inputfile = "udm_dataset.xml" - outputfile = "ord_dataset.pbtxt" + if not Path(args.input).is_file(): + logger.error("Conversion failed: --input file %s does not exist.", args.input) + raise SystemExit(1) - # Pull input filename from argument parameters - if kwargs["--input"]: - inputfile = kwargs["--input"] + udm_tree = ET.parse(args.input) # noqa: S314 -- UDM inputs are trusted local files, not untrusted network input. + udm_root = etree_to_dict(udm_tree.getroot()) + if "UDM" not in udm_root: + logger.error("Input file is not UDM format: expected a <UDM> root element.") + raise SystemExit(1) + udm_reactions: UdmDict = udm_root["UDM"] - # If the input file is not found, exit with an error message - if not os.path.isfile(inputfile): - logger.info( - "Error - Conversion failed. Please check that the --input file exists at the specified location." - ) - exit(1) - - # Load UDM file and parse XML into a dictionary - udm_tree = ET.parse(inputfile) - root = udm_tree.getroot() - # for child in root: - # print(child.tag, child.attrib) - udm_reactions = etree_to_dict(root) - - # <UDM> is expected to be the root element of the UDM file - if it isn't, we exit with an error. - if "UDM" not in udm_reactions: - logger.info( - "Error - Input file is not UDM format - please ensure the <UDM> tag is used as the root of your " - ".xml file." - ) - exit(1) - - # Access the data in the .xml file. - udm_reactions = udm_reactions["UDM"] - - # Determine dataset name and description - if given, the <TITLE> will be used as the name. - # If a global DOI is given, this will be included in the dataset description. - dataset_name = "" - dataset_description = "" - if "LEGAL" in udm_reactions: - if "TITLE" in udm_reactions["LEGAL"]: - outputfile = udm_reactions["LEGAL"]["TITLE"] + ".pbtxt" - dataset_name = udm_reactions["LEGAL"]["TITLE"] - if "DOI" in udm_reactions["LEGAL"]: - dataset_description = "UDM dataset DOI: " + udm_reactions["LEGAL"]["DOI"] - - # Set the dataset name if explicitly provided (overrides what's in the UDM file) - if kwargs["--name"]: - dataset_name = kwargs["--name"] - - # Set the dataset description if explicitly provided (overrides what's in the UDM file) - if kwargs["--description"]: - dataset_description = kwargs["--description"] - - # Set the output file name if explicitly provided (overrides what's in the UDM file) - if kwargs["--output"]: - outputfile = kwargs["--output"] - - # Inspect UDM reactions - # print(str(udm_reactions)) - - # List of ORD reactions-to-be - ord_reactions = [] - pb2_reactions = [] - - # This is unlikely to be hit, but just in case if "REACTIONS" not in udm_reactions: - logger.info("Error - <REACTIONS> element not found in input .xml file.") - exit(1) - - # If there is just one reaction in the entire file, we have to change our approach. - # Create a list of every reaction in the file. - udm_reactions_list = [] - if isinstance(udm_reactions["REACTIONS"]["REACTION"], dict): - udm_reactions_list.append(udm_reactions["REACTIONS"]["REACTION"]) - else: - for reaction in udm_reactions["REACTIONS"]["REACTION"]: - udm_reactions_list.append(reaction) + logger.error("No <REACTIONS> element found in input .xml file.") + raise SystemExit(1) + if "MOLECULES" not in udm_reactions: + logger.error("No <MOLECULES> element found in input .xml file.") + raise SystemExit(1) + + legal: UdmDict = udm_reactions.get("LEGAL", {}) + dataset_name = args.name or legal.get("TITLE", "") + dataset_description = args.description or ( + f"UDM dataset DOI: {legal['DOI']}" if "DOI" in legal else "" + ) + outputfile = args.output or ( + f"{legal['TITLE']}.pbtxt" if "TITLE" in legal else "ord_dataset.pbtxt" + ) - all_molecules = {} - for molecule in udm_reactions["MOLECULES"]["MOLECULE"]: - mol_data = dict() - mol_data["name"] = molecule["NAME"] + all_molecules: dict[str, UdmDict] = {} + for molecule in _as_list(udm_reactions["MOLECULES"]["MOLECULE"]): + mol_data: UdmDict = {"name": molecule.get("NAME")} if "MOLSTRUCTURE" in molecule: mol_data["molblock"] = molecule["MOLSTRUCTURE"] all_molecules[molecule["@ID"]] = mol_data - # Loop over each UDM reaction. UDM reactions may have one or more Variations. - # We will treat each Variation as a separate ORD Reaction. - # May be 1 or multiple, with null checks - # Return errors if UDM format is wrong - # Return warnings if data has to be omitted if not supported in ORD - for reaction in udm_reactions_list: - # Create a blank dictionary for each reaction. - pb2_reaction = reaction_pb2.Reaction() - - # Step 1 of 9: Identifiers - # Used to identify molecules - - pb2_inputs = [] - - if "REACTANT_ID" in reaction: - molinput = pb2_reaction.inputs[""] - reactant_list = reaction["REACTANT_ID"] - for reactant_id in reactant_list: - molcomponent = molinput.components.add() - molcomponent.identifiers.add( - type="CUSTOM", details="REACTANT_ID from UDM" - ) - molcomponent.identifiers[0].value = reactant_id - - # May be multiple RXNSTRUCTs - if "RXNSTRUCTURE" in reaction: - rxnstructures = [] - if isinstance(reaction["RXNSTRUCTURE"], dict): - rxnstructures.append(reaction["RXNSTRUCTURE"]) - else: - for structure in reaction["RXNSTRUCTURE"]: - rxnstructures.append(structure) - - for structure in rxnstructures: - udmformat = 1 - if "format" in structure: - udmformat = structure["format"] - - # Default reaction identifier type will be unspecified. - ordtype = 0 - orddetails = "" - - # For types not supported in ORD, the type will be CUSTOM - # and the details will contain the name of the type. - if udmformat == "cdxml": - ordtype = 1 - orddetails = "cdxml" - elif udmformat == "rinchi": - ordtype = 5 - elif udmformat == "rsmiles": - ordtype = 2 - elif udmformat == "rxn": - ordtype = 1 - orddetails = "rxn" - - udmvalue = None - if "value" in structure: - udmvalue = structure["value"] - - pb2_reaction.identifiers.add( - type=ordtype, details=orddetails, value=udmvalue - ) - - # Step 2 of 9: Inputs - # Used for reaction inputs, i.e. reactants, catalysts, solvents etc. - - # Reactions in UDM are more VARIATIONS on a reaction conducted during EXPERIMENTS - # Roles for Compounds and Products together in one enum for ORD. - # UNSPECIFIED, REACTANT, REAGENT, SOLVENT, CATALYST, WORKUP, INTERNAL_STANDARD, AUTHENTIC_STANDARD - # PRODUCT, BYPRODUCT, SIDE_PRODUCT - # Of these, REACTANT, REAGENT, SOLVENT, CATALYST, and PRODUCT are represented in UDM as separate data structures. - # They allow MULTIPLE of each one in each reaction with MULTIPLE compounds. - ord_inputs = [] - - # If there is one variation, it is a dict - variations = [] - if "VARIATION" in reaction: - if isinstance(reaction["VARIATION"], dict): - variations.append(reaction["VARIATION"]) - else: - for variation in reaction["VARIATION"]: - variations.append(variation) - - # If there are multiple variations, each will be its own reaction. - for variation in variations: + pb2_reactions = [] + for reaction in _as_list(udm_reactions["REACTIONS"]["REACTION"]): + base_reaction = _build_base_reaction(reaction) + # Each variation becomes its own Reaction, seeded with the + # reaction-level identifiers built above. A reaction with no + # <VARIATION> elements still gets a single Reaction, since it has no + # variation-specific data to add. + for variation in _as_list(reaction.get("VARIATION")) or [{}]: pb2_reaction = reaction_pb2.Reaction() + pb2_reaction.CopyFrom(base_reaction) + _add_variation( + pb2_reaction, reaction, variation, udm_reactions, all_molecules + ) + pb2_reactions.append(pb2_reaction) - reactants = [] - if "REACTANT" in variation: - if isinstance(variation["REACTANT"], dict): - reactants.append(variation["REACTANT"]) - else: - for reactant in variation["REACTANT"]: - reactants.append(reactant) - - reagents = [] - if "REAGENT" in variation: - if isinstance(variation["REAGENT"], dict): - reagents.append(variation["REAGENT"]) - else: - for reagent in variation["REAGENT"]: - reagents.append(reagent) - - catalysts = [] - if "CATALYST" in variation: - if isinstance(variation["CATALYST"], dict): - catalysts.append(variation["CATALYST"]) - else: - for catalyst in variation["CATALYST"]: - catalysts.append(catalyst) - - solvents = [] - if "SOLVENT" in variation: - if isinstance(variation["SOLVENT"], dict): - solvents.append(variation["SOLVENT"]) - else: - for solvent in variation["SOLVENT"]: - solvents.append(solvent) - - for reactant in reactants: - pb2_compound = reaction_pb2.Compound() - compound = dict() - # TODO: Check the following translation from MOLECULE to identifiers. - compound["identifiers"] = reactant["MOLECULE"] - # compound["amount"] = reactant["AMOUNT"] - compound["reaction_role"] = [] - compound["is_limiting"] = [] - compound["preparations"] = [] - compound["source"] = [] - compound["features"] = [] - compound["analyses"] = [] - compound["texture"] = [] - - if "MOLECULE" in reactant: - molval = all_molecules.get(reactant["MOLECULE"]["@MOL_ID"]) - # pb2_reaction.identifiers.add(type=0, details='', value=molval) - molinput = pb2_reaction.inputs[reactant["MOLECULE"]["@MOL_ID"]] - molcomponent = molinput.components.add() - if "molblock" in molval: - molcomponent.identifiers.add( - type="MOLBLOCK", details="MOLECULE -> MOLSTRUCTURE from UDM" - ) - molcomponent.identifiers[0].value = molval["molblock"] - else: - molcomponent.identifiers.add(type="CUSTOM") - if molval is not None: - molcomponent.identifiers[0].value = molval["name"] - if "MOLECULE" in reactant and "NAME" in reactant["MOLECULE"]: - pb2_compound.identifiers.add(value=reactant["MOLECULE"]["NAME"]) - if "AMOUNT" in reactant: - convertedAmount = float(reactant["AMOUNT"]) - pb2_compound.amount.mass.value = convertedAmount - - pb2_components = [] - pb2_components.append(pb2_compound) - - pb2_input = reaction_pb2.ReactionInput() - pb2_input.components.append(pb2_compound) - pb2_inputs.append(pb2_input) - - # pb2_reaction.inputs.update(key=pb2_input, value=pb2_input) - # pb2_input.components.add(pb2_compound) - # pb2_reaction.inputs.update(key=reactant["MOLECULE"]["@MOL_ID"], value=pb2_input) - # updates.update_reaction(pb2_reaction) - - # Step 4 of 9: Conditions - # Describes the reaction conditions. - - for reagent in reagents: - if "MOLECULE" in reagent: - molval = all_molecules.get(reagent["MOLECULE"]["@MOL_ID"]) - # print(molval) - # pb2_reaction.identifiers.add(type=0, details='', value=molval) - molinput = pb2_reaction.inputs[reagent["MOLECULE"]["@MOL_ID"]] - molcomponent = molinput.components.add() - if "molblock" in molval: - molcomponent.identifiers.add( - type="MOLBLOCK", details="MOLECULE -> MOLSTRUCTURE from UDM" - ) - molcomponent.identifiers[0].value = molval["molblock"] - else: - molcomponent.identifiers.add(type="CUSTOM") - if molval["name"] is not None: - molcomponent.identifiers[0].value = molval["name"] - molcomponent.reaction_role = reaction_pb2.ReactionRole.REAGENT - - for catalyst in catalysts: - if "MOLECULE" in catalyst: - molval = all_molecules.get(catalyst["MOLECULE"]["@MOL_ID"]) - # print(molval) - # pb2_reaction.identifiers.add(type=0, details='', value=molval) - molinput = pb2_reaction.inputs[catalyst["MOLECULE"]["@MOL_ID"]] - molcomponent = molinput.components.add() - if "molblock" in molval: - molcomponent.identifiers.add( - type="MOLBLOCK", details="MOLECULE -> MOLSTRUCTURE from UDM" - ) - molcomponent.identifiers[0].value = molval["molblock"] - else: - molcomponent.identifiers.add(type="CUSTOM") - if molval["name"] is not None: - molcomponent.identifiers[0].value = molval["name"] - molcomponent.reaction_role = reaction_pb2.ReactionRole.CATALYST - - for solvent in solvents: - if "MOLECULE" in solvent: - molval = all_molecules.get(solvent["MOLECULE"]["@MOL_ID"]) - # print(molval) - # pb2_reaction.identifiers.add(type=0, details='', value=molval) - molinput = pb2_reaction.inputs[solvent["MOLECULE"]["@MOL_ID"]] - molcomponent = molinput.components.add() - if "molblock" in molval: - molcomponent.identifiers.add( - type="MOLBLOCK", details="MOLECULE -> MOLSTRUCTURE from UDM" - ) - molcomponent.identifiers[0].value = molval["molblock"] - else: - molcomponent.identifiers.add(type="CUSTOM") - if molval["name"] is not None: - molcomponent.identifiers[0].value = molval["name"] - molcomponent.reaction_role = reaction_pb2.ReactionRole.SOLVENT - - if ( - "CONDITIONS" in variation - and "CONDITION_GROUP" in variation["CONDITIONS"] - ): - pb2_conditions = reaction_pb2.ReactionConditions() - if "TEMPERATURE" in variation["CONDITIONS"]["CONDITION_GROUP"]: - if ( - "exact" - in variation["CONDITIONS"]["CONDITION_GROUP"]["TEMPERATURE"] - ): - pb2_reaction.conditions.temperature.setpoint.value = float( - variation["CONDITIONS"]["CONDITION_GROUP"]["TEMPERATURE"][ - "exact" - ] - ) - if "PRESSURE" in variation["CONDITIONS"]["CONDITION_GROUP"]: - if ( - "exact" - in variation["CONDITIONS"]["CONDITION_GROUP"]["PRESSURE"] - ): - pb2_reaction.conditions.pressure.setpoint.value = float( - variation["CONDITIONS"]["CONDITION_GROUP"]["PRESSURE"][ - "exact" - ] - ) - if "STIRRING" in variation["CONDITIONS"]["CONDITION_GROUP"]: - pb2_reaction.conditions.stirring.details = variation["CONDITIONS"][ - "CONDITION_GROUP" - ]["STIRRING"] - if "REFLUX" in variation["CONDITIONS"]["CONDITION_GROUP"]: - pb2_reaction.conditions.reflux = variation["CONDITIONS"][ - "CONDITION_GROUP" - ]["REFLUX"] - if "PH" in variation["CONDITIONS"]["CONDITION_GROUP"]: - if "exact" in variation["CONDITIONS"]["CONDITION_GROUP"]["PH"]: - pb2_reaction.conditions.ph = float( - variation["CONDITIONS"]["CONDITION_GROUP"]["PH"]["exact"] - ) - - pb2_setup = reaction_pb2.ReactionSetup() - if "PREPARATION" in variation["CONDITIONS"]["CONDITION_GROUP"]: - pb2_reaction.setup.environment = variation["CONDITIONS"][ - "CONDITION_GROUP" - ]["PREPARATION"] - - # pb2_reaction.conditions = pb2_conditions - - # Step 3 of 9: Setup - # Describes the reaction setup. - - pb2_setup = reaction_pb2.ReactionSetup() - # pb2_reaction.setup = pb2_setup - - # Step 5 of 9: Notes - # Extra notes about the reaction. - - pb2_notes = reaction_pb2.ReactionNotes() - # pb2_notes.safety_notes - # pb2_notes.procedure_details - pb2_reaction.notes.procedure_details = "" - - # Step 6 of 9: Observations - # Notes about observations during the reaction. - - pb2_observations = reaction_pb2.ReactionObservation() - # pb2_observations.time - if "VARIATION" in reaction and "COMMENT" in reaction["VARIATION"]: - pb2_observations.comment = reaction["VARIATION"]["COMMENT"] - pb2_reaction.observations.append(pb2_observations) - - # Step 7 of 9: Workups - # Reaction workups - - pb2_workups = reaction_pb2.ReactionWorkup() - # type, details, duration, input, amount, temperature, stirring, target_ph - pb2_reaction.workups.append(pb2_workups) - - # Step 8 of 9: Outcomes - # The outcomes of the reaction - time taken, the products etc. - - pb2_outcomes = reaction_pb2.ReactionOutcome() - # reaction_time, conversion, analyses - - products = [] - - if "PRODUCT_ID" in reaction: - molinput = pb2_reaction.inputs[""] - product_list = reaction["PRODUCT_ID"] - outcome = pb2_reaction.outcomes.add() - for product_id in product_list: - molcomponent = outcome.products.add() - molcomponent.identifiers.add( - type="CUSTOM", details="PRODUCT_ID from UDM" - ) - molcomponent.identifiers[0].value = product_id - - if "VARIATION" in reaction and "PRODUCT" in reaction["VARIATION"]: - if isinstance(reaction["VARIATION"]["PRODUCT"], dict): - products.append(reaction["VARIATION"]["PRODUCT"]) - else: - for product in reaction["VARIATION"]["PRODUCT"]: - products.append(product) - - for udm_product in products: - molecule = udm_product["MOLECULE"] - outcome = pb2_reaction.outcomes.add() - product = outcome.products.add() - if all_molecules.get(molecule["@MOL_ID"]) is not None: - molval = all_molecules.get(molecule["@MOL_ID"]) - if "molblock" in molval: - product.identifiers.add( - type="MOLBLOCK", details="MOLECULE -> MOLSTRUCTURE from UDM" - ) - product.identifiers[0].value = molval["molblock"] - else: - product.identifiers.add(type="CUSTOM") - if molval["name"] is not None: - product.identifiers[0].value = molval["name"] - if "YIELD" in udm_product: - product_measurement = product.measurements.add() - product_measurement.type = ( - reaction_pb2.ProductMeasurement.ProductMeasurementType.YIELD - ) - product_measurement.float_value.value = float( - udm_product["YIELD"]["exact"] - ) - - # Step 9 of 9: Provenance - # Publication and patent details, attribution, other metadata - - ord_provenance = dict() - pb2_provenance = reaction_pb2.ReactionProvenance() - ord_provenance_experimenter = dict() - if "LEGAL" in udm_reactions: - pb2_reaction.provenance.experimenter.organization = udm_reactions["LEGAL"][ - "PRODUCER" - ] - if "VARIATION" in reaction and "SCIENTIST" in reaction["VARIATION"]: - pb2_reaction.provenance.experimenter.name = reaction["VARIATION"][ - "SCIENTIST" - ] - if "LEGAL" in udm_reactions: - pb2_reaction.provenance.record_created.person.organization = udm_reactions[ - "LEGAL" - ]["PRODUCER"] - if "VARIATION" in reaction and "SCIENTIST" in reaction["VARIATION"]: - pb2_reaction.provenance.record_created.person.name = reaction["VARIATION"][ - "SCIENTIST" - ] - if "ORGANISATIONS" in reaction: - pb2_reaction.provenance.city = reaction["ORGANISATIONS"][0]["ORGANISATION"][ - "ADDRESS" - ] - - # experiment start - - if ( - "LEGAL" in udm_reactions - and "DOI" in udm_reactions["LEGAL"] - and "CITATIONS" not in reaction - ): - pb2_reaction.provenance.doi = udm_reactions["LEGAL"]["DOI"] - elif ( - "CITATIONS" in udm_reactions - and "VARIATION" in reaction - and "CITATION" in reaction["VARIATION"] - ): - variation_citation = reaction["VARIATION"]["CITATION"] - variation_doi = "" - for citation in udm_reactions["CITATIONS"]["CITATION"]: - if isinstance(variation_citation, list): - variation_citation = variation_citation[0] - if ( - citation["@ID"] == variation_citation["@CIT_ID"] - and "DOI" in citation - ): - variation_doi = citation["DOI"] - if variation_doi != "": - pb2_reaction.provenance.doi = variation_doi - - if "CITATIONS" in reaction: - pb2_reaction.provenance.doi = reaction["CITATIONS"][0]["CITATION"]["DOI"] - pb2_reaction.provenance.patent = reaction["CITATIONS"][0]["CITATION"][ - "PATENT_NUMBER" - ] - - # publication url is not provided - - if "VARIATION" in reaction and "CREATION_DATE" in reaction["VARIATION"]: - pb2_reaction.provenance.record_created.time = reaction["VARIATION"][ - "CREATION_DATE" - ] - - if "VARIATION" in reaction and "MODIFICATION_DATE" in reaction["VARIATION"]: - pb2_reaction.provenance.record_modified.time = reaction["VARIATION"][ - "MODIFICATION_DATE" - ] - - # ord_provenance["reaction_metadata"] = "" - pb2_reaction.provenance.is_mined = False - - # Generate REACTION ID - pb2_reaction.reaction_id = generate_reaction_id() - - pb2_reactions.append(pb2_reaction) - - # Create the ORD dataset dataset = dataset_pb2.Dataset( name=dataset_name, description=dataset_description, reactions=pb2_reactions ) - - # Assign canonical dataset_id/reaction_ids and provenance updates. + # Assigns canonical dataset_id/reaction_ids and provenance updates. updates.update_dataset(dataset) - # Validate the dataset - if not kwargs["--no-validate"]: + if not args.no_validate: validations.validate_datasets({"_COMBINED": dataset}) - # Create the .pbtxt file message_helpers.save_message(dataset, outputfile) + logger.info("Conversion completed successfully: wrote %s.", outputfile) + + +def _as_list(value: Any) -> list[UdmDict]: + """Normalizes an etree_to_dict value that may be absent, a dict, or a list.""" + if not value: + return [] + if isinstance(value, dict): + return [value] + return value + + +def _build_base_reaction(reaction: UdmDict) -> reaction_pb2.Reaction: + """Builds the reaction-level data shared by every variation of `reaction`. + + Covers the UDM fields that live directly on <REACTION> rather than on a + specific <VARIATION>: REACTANT_ID/PRODUCT_ID placeholders and RXNSTRUCTURE + identifiers. + """ + base_reaction = reaction_pb2.Reaction() + + if "REACTANT_ID" in reaction: + molinput = base_reaction.inputs["REACTANT_IDS"] + for reactant_id in reaction["REACTANT_ID"]: + identifier = molinput.components.add().identifiers.add( + type="CUSTOM", details="REACTANT_ID from UDM" + ) + identifier.value = reactant_id + + for structure in _as_list(reaction.get("RXNSTRUCTURE")): + udm_format = structure.get("format", 1) + ordtype, orddetails = _RXNSTRUCTURE_FORMATS.get( + udm_format, (reaction_pb2.ReactionIdentifier.UNSPECIFIED, "") + ) + if ( + "value" in structure + and ordtype != reaction_pb2.ReactionIdentifier.UNSPECIFIED + ): + base_reaction.identifiers.add( + type=ordtype, details=orddetails, value=structure["value"] + ) + + if "PRODUCT_ID" in reaction: + outcome = base_reaction.outcomes.add() + for product_id in reaction["PRODUCT_ID"]: + identifier = outcome.products.add().identifiers.add( + type="CUSTOM", details="PRODUCT_ID from UDM" + ) + identifier.value = product_id + + return base_reaction + + +def _add_variation( + pb2_reaction: reaction_pb2.Reaction, + reaction: UdmDict, + variation: UdmDict, + udm_reactions: UdmDict, + all_molecules: dict[str, UdmDict], +) -> None: + """Populates a Reaction already seeded with base data from one VARIATION.""" + for role_name, role in _ROLES.items(): + for entry in _as_list(variation.get(role_name)): + _add_component(pb2_reaction, all_molecules, entry, role, role_name) + + condition_group: UdmDict = variation.get("CONDITIONS", {}).get( + "CONDITION_GROUP", {} + ) + if condition_group: + _add_conditions(pb2_reaction, condition_group) + + comment = variation.get("COMMENT") + if comment: + pb2_reaction.observations.add().comment = comment + + for udm_product in _as_list(variation.get("PRODUCT")): + _add_product(pb2_reaction, all_molecules, udm_product) + + _add_provenance(pb2_reaction, reaction, variation, udm_reactions) + pb2_reaction.provenance.is_mined = False + + +def _add_component( + pb2_reaction: reaction_pb2.Reaction, + all_molecules: dict[str, UdmDict], + entry: UdmDict, + role: int, + role_label: str, +) -> None: + """Adds one UDM REACTANT/REAGENT/CATALYST/SOLVENT entry as a ReactionInput.""" + if "MOLECULE" not in entry: + return + mol_id = entry["MOLECULE"]["@MOL_ID"] + component = pb2_reaction.inputs[mol_id].components.add() + _set_molecule_identifier(component, all_molecules, mol_id, role_label) + component.reaction_role = role + if "AMOUNT" in entry: + # UDM's parsed <AMOUNT> is a bare number with no unit attribute visible + # in the source data available at the time of writing; grams are + # assumed. Revisit once real UDM sample data confirms the unit. + component.amount.mass.value = float(entry["AMOUNT"]) + component.amount.mass.units = reaction_pb2.Mass.GRAM + + +def _set_molecule_identifier( + component: reaction_pb2.Compound | reaction_pb2.ProductCompound, + all_molecules: dict[str, UdmDict], + mol_id: str, + role_label: str, +) -> None: + """Sets a CompoundIdentifier on `component`, falling back if `mol_id` is unknown.""" + molval = all_molecules.get(mol_id) + if molval and molval.get("molblock"): + identifier = component.identifiers.add( + type="MOLBLOCK", details=_MOLBLOCK_DETAILS + ) + identifier.value = molval["molblock"] + elif molval and molval.get("name"): + identifier = component.identifiers.add( + type="CUSTOM", details=f"{role_label} MOLECULE NAME from UDM" + ) + identifier.value = molval["name"] + else: + identifier = component.identifiers.add( + type="CUSTOM", + details=f"{role_label} MOL_ID from UDM (not found in <MOLECULES>)", + ) + identifier.value = mol_id + + +def _add_conditions( + pb2_reaction: reaction_pb2.Reaction, condition_group: UdmDict +) -> None: + """Populates ReactionConditions/ReactionSetup from a UDM CONDITION_GROUP.""" + temperature = condition_group.get("TEMPERATURE", {}) + if "exact" in temperature: + # No unit is present in the source data available at the time of + # writing, so Celsius is assumed. + pb2_reaction.conditions.temperature.setpoint.value = float(temperature["exact"]) + pb2_reaction.conditions.temperature.setpoint.units = ( + reaction_pb2.Temperature.CELSIUS + ) - # Write JSON file for debug - debugfile = "ORD_UDM_CONVERSION_" + inputfile + ".debug.json" - f = open(debugfile, "w") - f.write(json.dumps({}, indent=4, separators=(", ", ": "))) - f.close() + pressure = condition_group.get("PRESSURE", {}) + if "exact" in pressure: + # Same caveat as temperature: unit assumed, not present in the source data. + pb2_reaction.conditions.pressure.setpoint.value = float(pressure["exact"]) + pb2_reaction.conditions.pressure.setpoint.units = ( + reaction_pb2.Pressure.ATMOSPHERE + ) - # Successfully converted UDM .xml file into ORD dataset! - logger.info( - "Conversion completed successfully! Check ORD_UDM_CONVERSION_" - + inputfile - + ".debug.json file for errors." - ) + if "STIRRING" in condition_group: + pb2_reaction.conditions.stirring.type = reaction_pb2.StirringConditions.CUSTOM + pb2_reaction.conditions.stirring.details = condition_group["STIRRING"] + + if "REFLUX" in condition_group: + pb2_reaction.conditions.reflux = _parse_bool(condition_group["REFLUX"]) + + ph = condition_group.get("PH", {}) + if "exact" in ph: + pb2_reaction.conditions.ph = float(ph["exact"]) - exit(0) + if "PREPARATION" in condition_group: + pb2_reaction.setup.environment.type = ( + reaction_pb2.ReactionSetup.ReactionEnvironment.CUSTOM + ) + pb2_reaction.setup.environment.details = condition_group["PREPARATION"] + + +def _parse_bool(value: str) -> bool: + """Parses a UDM boolean-ish string (e.g. "true"/"false"/"1"/"0").""" + return str(value).strip().lower() in ("true", "1", "yes") + + +def _add_product( + pb2_reaction: reaction_pb2.Reaction, + all_molecules: dict[str, UdmDict], + udm_product: UdmDict, +) -> None: + """Adds one UDM VARIATION/PRODUCT entry as a ReactionOutcome product.""" + molecule = udm_product.get("MOLECULE") + if not molecule: + return + component = pb2_reaction.outcomes.add().products.add() + _set_molecule_identifier( + component, all_molecules, molecule.get("@MOL_ID"), "PRODUCT" + ) + yield_ = udm_product.get("YIELD", {}) + if "exact" in yield_: + measurement = component.measurements.add() + measurement.type = reaction_pb2.ProductMeasurement.ProductMeasurementType.YIELD + measurement.float_value.value = float(yield_["exact"]) + + +def _add_provenance( + pb2_reaction: reaction_pb2.Reaction, + reaction: UdmDict, + variation: UdmDict, + udm_reactions: UdmDict, +) -> None: + """Populates ReactionProvenance from reaction- and variation-level UDM fields.""" + legal: UdmDict = udm_reactions.get("LEGAL", {}) + producer = legal.get("PRODUCER") + if producer: + pb2_reaction.provenance.experimenter.organization = producer + pb2_reaction.provenance.record_created.person.organization = producer + + scientist = variation.get("SCIENTIST") + if scientist: + pb2_reaction.provenance.experimenter.name = scientist + pb2_reaction.provenance.record_created.person.name = scientist + + if "ORGANISATIONS" in reaction: + pb2_reaction.provenance.city = reaction["ORGANISATIONS"][0]["ORGANISATION"][ + "ADDRESS" + ] + + doi = legal.get("DOI") + if doi and "CITATIONS" not in reaction: + pb2_reaction.provenance.doi = doi + elif "CITATIONS" in udm_reactions and "CITATION" in variation: + variation_citation = variation["CITATION"] + if isinstance(variation_citation, list): + variation_citation = variation_citation[0] + for citation in _as_list(udm_reactions["CITATIONS"]["CITATION"]): + if ( + citation.get("@ID") == variation_citation.get("@CIT_ID") + and "DOI" in citation + ): + pb2_reaction.provenance.doi = citation["DOI"] + break + + if "CITATIONS" in reaction: + reaction_citation = _as_list(reaction["CITATIONS"])[0]["CITATION"] + if "DOI" in reaction_citation: + pb2_reaction.provenance.doi = reaction_citation["DOI"] + if "PATENT_NUMBER" in reaction_citation: + pb2_reaction.provenance.patent = reaction_citation["PATENT_NUMBER"] + + creation_date = variation.get("CREATION_DATE") + if creation_date: + pb2_reaction.provenance.record_created.time.value = creation_date + + modification_date = variation.get("MODIFICATION_DATE") + if modification_date: + event = pb2_reaction.provenance.record_modified.add() + event.time.value = modification_date + if scientist: + event.person.name = scientist + if producer: + event.person.organization = producer # Converts XML tree into Python dictionary. # https://stackoverflow.com/questions/7684333/converting-xml-to-dictionary-using-elementtree -def etree_to_dict(t): - d = {t.tag: {} if t.attrib else None} +def etree_to_dict(t: ET.Element) -> UdmDict: + """Recursively converts an ElementTree Element into a nested dict.""" + d: UdmDict = {t.tag: {} if t.attrib else None} children = list(t) if children: dd = defaultdict(list) @@ -618,31 +416,5 @@ def etree_to_dict(t): return d -def generate_reaction_id(): - random_id_parts = [ - "0", - "1", - "2", - "3", - "4", - "5", - "6", - "7", - "8", - "9", - "a", - "b", - "c", - "d", - "e", - "f", - ] - new_reaction_id = "ord-" - for i in range(0, 31): - new_reaction_id += random.choice(random_id_parts) - return new_reaction_id - - -# Shows the Usage information if parameters not provided (self documenting) if __name__ == "__main__": - main(docopt.docopt(__doc__)) + main(parse_args()) diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py new file mode 100644 index 00000000..d8ab317a --- /dev/null +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -0,0 +1,208 @@ +# Copyright 2025 Open Reaction Database Project Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Tests for ord_schema.scripts.convert_udm_to_ord.""" + +import pathlib + +import pytest + +from ord_schema import message_helpers +from ord_schema.proto import dataset_pb2 +from ord_schema.scripts import convert_udm_to_ord + +_MOLECULES = """ + <MOLECULES> + <MOLECULE ID="m1"><NAME>Reactant A</NAME></MOLECULE> + <MOLECULE ID="m2"><NAME>Product A</NAME></MOLECULE> + </MOLECULES> +""" + +_ONE_VARIATION_REACTION = """ + <REACTION> + <VARIATION> + <SCIENTIST>Test Scientist</SCIENTIST> + <CREATION_DATE>2020-01-01</CREATION_DATE> + <REACTANT> + <MOLECULE MOL_ID="m1"><NAME>Reactant A</NAME></MOLECULE> + <AMOUNT>1.0</AMOUNT> + </REACTANT> + <PRODUCT> + <MOLECULE MOL_ID="m2"><NAME>Product A</NAME></MOLECULE> + <YIELD><exact>85.0</exact></YIELD> + </PRODUCT> + <CONDITIONS> + <CONDITION_GROUP> + <TEMPERATURE><exact>25.0</exact></TEMPERATURE> + </CONDITION_GROUP> + </CONDITIONS> + <COMMENT>A test reaction</COMMENT> + </VARIATION> + </REACTION> +""" + + +def _udm_xml(reactions_xml: str) -> str: + """Wraps a <REACTION>...</REACTION> fragment in a minimal UDM document.""" + return f"""<UDM> + <LEGAL> + <TITLE>Test Dataset + Test Org + 10.0000/test-doi + + {_MOLECULES} + + {reactions_xml} + + +""" + + +def _write(tmp_path: pathlib.Path, filename: str, content: str) -> str: + path = tmp_path / filename + path.write_text(content) + return str(path) + + +def test_simple(tmp_path): + input_filename = _write(tmp_path, "input.xml", _udm_xml(_ONE_VARIATION_REACTION)) + output_filename = str(tmp_path / "output.pbtxt") + # UDM's SCIENTIST field carries only a name, no email, so a person's + # email -- required by ORD validation -- can never be filled in from UDM + # source data alone. Validation is disabled here for that known reason. + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + assert dataset.name == "Test Dataset" + assert dataset.description == "UDM dataset DOI: 10.0000/test-doi" + assert len(dataset.reactions) == 1 + + reaction = dataset.reactions[0] + assert reaction.provenance.experimenter.name == "Test Scientist" + assert reaction.provenance.record_created.time.value == "2020-01-01" + assert reaction.conditions.temperature.setpoint.value == 25.0 + assert ( + reaction.conditions.temperature.setpoint.units + == reaction.conditions.temperature.setpoint.CELSIUS + ) + assert reaction.observations[0].comment == "A test reaction" + + [component] = reaction.inputs["m1"].components + assert component.amount.mass.value == 1.0 + assert component.amount.mass.units == component.amount.mass.GRAM + + [outcome] = reaction.outcomes + [product] = outcome.products + assert product.identifiers[0].value == "Product A" + [measurement] = product.measurements + assert measurement.float_value.value == 85.0 + + +def test_multiple_variations_become_separate_reactions(tmp_path): + """Regression test: each must produce its own Reaction. + + A prior version of this script reset the in-progress Reaction object on + every VARIATION and only kept the last one, silently discarding every + other variation's data. + """ + reaction_xml = """ + + + Scientist One + + Reactant A + 1.0 + + + Product A + + + + Scientist Two + + Reactant A + 2.0 + + + Product A + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + assert len(dataset.reactions) == 2 + scientists = {r.provenance.experimenter.name for r in dataset.reactions} + assert scientists == {"Scientist One", "Scientist Two"} + amounts = { + r.inputs["m1"].components[0].amount.mass.value for r in dataset.reactions + } + assert amounts == {1.0, 2.0} + + +def test_reaction_without_variation_still_emits_one_reaction(tmp_path): + reaction_xml = """ + + + rsmiles + CC.O>>CCO + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + assert len(dataset.reactions) == 1 + assert dataset.reactions[0].identifiers[0].value == "CC.O>>CCO" + + +def test_unknown_molecule_reference_does_not_crash(tmp_path): + reaction_xml = """ + + + + Missing + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + [component] = dataset.reactions[0].inputs["does_not_exist"].components + assert component.identifiers[0].type == component.identifiers[0].CUSTOM + assert component.identifiers[0].value == "does_not_exist" + + +def test_missing_input_file(tmp_path): + argv = ["--input", str(tmp_path / "does_not_exist.xml")] + with pytest.raises(SystemExit): + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + +def test_not_udm_format(tmp_path): + input_filename = _write(tmp_path, "input.xml", "") + argv = ["--input", input_filename] + with pytest.raises(SystemExit): + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) diff --git a/pyproject.toml b/pyproject.toml index f77d599e..2363e5fa 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -221,15 +221,6 @@ convention = "google" "**/conftest.py" = ["D"] # Tutorial notebooks legitimately show commented-out alternative code. "**/*.ipynb" = ["ERA001", "S310", "D"] # tutorials download from known URLs; D: cells are narrative, not modules -# convert_udm_to_ord.py is a straight port of a rough-draft external contribution; -# it is functional but has known gaps (dead code, missing annotations/docstrings, -# unvetted XML/random usage) tracked as follow-up work rather than fixed here. -"**/scripts/convert_udm_to_ord.py" = [ - "ANN", "D103", "B007", "C408", "E501", "ERA001", "F841", "G003", - "N806", "PERF402", "PLR1722", "PTH113", "PTH123", "S311", "S314", - "SIM115", "TD002", -] [tool.ty.src] -# convert_udm_to_ord.py: see the ruff per-file-ignore above for why. -exclude = ["**/*_pb2.py*", "**/scripts/convert_udm_to_ord.py"] +exclude = ["**/*_pb2.py*"] From 682ed305537ead74740735b206a2643d29629950 Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 15:23:02 +0100 Subject: [PATCH 03/15] Add a submitter --email flag, and ground the converter in the real UDM 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. - 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. - is a child of , not as the code assumed; moved to the correct level. can also repeat , which the code didn't handle (would have crashed calling dict methods on a list). - 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 --- ord_schema/scripts/convert_udm_to_ord.py | 209 ++++++++++++++---- ord_schema/scripts/convert_udm_to_ord_test.py | 137 ++++++++++-- 2 files changed, 285 insertions(+), 61 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index a1ea7819..6f82382b 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -17,9 +17,13 @@ may contain multiple elements; each variation is emitted as its own ORD Reaction, since UDM variations represent distinct experiments run under the same reaction identifiers. + +UDM (Unified Data Model) is a Pistoia Alliance format; see +https://github.com/PistoiaAlliance/UDM for the schema and example data. """ import argparse +import getpass import xml.etree.ElementTree as ET from collections import defaultdict from pathlib import Path @@ -54,6 +58,38 @@ "rxn": (reaction_pb2.ReactionIdentifier.CUSTOM, "rxn"), } +# Unit mappings below are taken from the UDM v6.0.0 XSD schema +# (udm_6_0_0_units.xsd, udm_6_0_0.xsd), not guessed: +# https://github.com/PistoiaAlliance/UDM/blob/master/udm_6_0_0_units.xsd + +# is UDM's molType: a decimal with an optional @unit attribute +# defaulting to "mol". Units not listed here fall back to UNSPECIFIED. +_MOLES_UNITS: dict[str, int] = { + "mol": reaction_pb2.Moles.MOLE, + "mmol": reaction_pb2.Moles.MILLIMOLE, + "umol": reaction_pb2.Moles.MICROMOLE, + "μmol": reaction_pb2.Moles.MICROMOLE, # UDM allows the literal mu character. + "nmol": reaction_pb2.Moles.NANOMOLE, +} + +# temperatureRange's @unit attribute defaults to "degC". +_TEMPERATURE_UNITS: dict[str, int] = { + "degC": reaction_pb2.Temperature.CELSIUS, + "degF": reaction_pb2.Temperature.FAHRENHEIT, + "K": reaction_pb2.Temperature.KELVIN, +} + +# pressureRange's @unit attribute defaults to "torr", not "atm". +_PRESSURE_UNITS: dict[str, int] = { + "atm": reaction_pb2.Pressure.ATMOSPHERE, + "bar": reaction_pb2.Pressure.BAR, + "psi": reaction_pb2.Pressure.PSI, + "KPa": reaction_pb2.Pressure.KILOPASCAL, + "Pa": reaction_pb2.Pressure.PASCAL, + "torr": reaction_pb2.Pressure.TORR, + "mmHg": reaction_pb2.Pressure.MM_HG, +} + def parse_args(argv: list[str] | None = None) -> argparse.Namespace: """Parses command-line arguments.""" @@ -62,6 +98,15 @@ def parse_args(argv: list[str] | None = None) -> argparse.Namespace: parser.add_argument("--output", help="Output Dataset filename (*.pbtxt)") parser.add_argument("--name", help="Name for this dataset") parser.add_argument("--description", help="Description for this dataset") + parser.add_argument( + "--email", + help=( + "Email address of the person running this conversion. Recorded as " + "the record_created/record_modified provenance, since that person " + "-- not the original UDM scientist, who is recorded separately as " + "the experimenter -- is who is creating this ORD record." + ), + ) parser.add_argument( "--no-validate", action="store_true", @@ -114,6 +159,11 @@ def main(args: argparse.Namespace) -> None: mol_data["molblock"] = molecule["MOLSTRUCTURE"] all_molecules[molecule["@ID"]] = mol_data + # The person running this conversion is the one creating this ORD record, + # distinct from the original UDM scientist who performed the reaction + # (recorded separately as the experimenter). + submitter_username = getpass.getuser() + pb2_reactions = [] for reaction in _as_list(udm_reactions["REACTIONS"]["REACTION"]): base_reaction = _build_base_reaction(reaction) @@ -125,7 +175,13 @@ def main(args: argparse.Namespace) -> None: pb2_reaction = reaction_pb2.Reaction() pb2_reaction.CopyFrom(base_reaction) _add_variation( - pb2_reaction, reaction, variation, udm_reactions, all_molecules + pb2_reaction, + reaction, + variation, + udm_reactions, + all_molecules, + submitter_username, + args.email, ) pb2_reactions.append(pb2_reaction) @@ -142,13 +198,32 @@ def main(args: argparse.Namespace) -> None: logger.info("Conversion completed successfully: wrote %s.", outputfile) -def _as_list(value: Any) -> list[UdmDict]: - """Normalizes an etree_to_dict value that may be absent, a dict, or a list.""" +def _as_list(value: Any) -> list[Any]: + """Normalizes an etree_to_dict value that may be absent, a single item, or a list. + + etree_to_dict collapses a single occurrence of a repeatable element to a + bare dict or string rather than a one-element list, so a naive `for x in + value` silently iterates a string's characters instead of treating it as + one item. Only an already-list value is left as-is. + """ if not value: return [] + if isinstance(value, list): + return value + return [value] + + +def _text_and_unit(value: Any) -> tuple[str | None, str | None]: + """Splits an etree_to_dict value into (text, unit). + + Several UDM elements (AMOUNT, PREPARATION) carry an optional @unit or + @format attribute alongside their text. etree_to_dict represents such an + element as plain text when no attribute is present, or as a dict with + "#text" plus "@..." keys when one is. This normalizes both shapes. + """ if isinstance(value, dict): - return [value] - return value + return value.get("#text"), value.get("@unit") or value.get("@format") + return value, None def _build_base_reaction(reaction: UdmDict) -> reaction_pb2.Reaction: @@ -162,7 +237,7 @@ def _build_base_reaction(reaction: UdmDict) -> reaction_pb2.Reaction: if "REACTANT_ID" in reaction: molinput = base_reaction.inputs["REACTANT_IDS"] - for reactant_id in reaction["REACTANT_ID"]: + for reactant_id in _as_list(reaction["REACTANT_ID"]): identifier = molinput.components.add().identifiers.add( type="CUSTOM", details="REACTANT_ID from UDM" ) @@ -183,7 +258,7 @@ def _build_base_reaction(reaction: UdmDict) -> reaction_pb2.Reaction: if "PRODUCT_ID" in reaction: outcome = base_reaction.outcomes.add() - for product_id in reaction["PRODUCT_ID"]: + for product_id in _as_list(reaction["PRODUCT_ID"]): identifier = outcome.products.add().identifiers.add( type="CUSTOM", details="PRODUCT_ID from UDM" ) @@ -198,16 +273,30 @@ def _add_variation( variation: UdmDict, udm_reactions: UdmDict, all_molecules: dict[str, UdmDict], + submitter_username: str, + submitter_email: str | None, ) -> None: """Populates a Reaction already seeded with base data from one VARIATION.""" for role_name, role in _ROLES.items(): for entry in _as_list(variation.get(role_name)): _add_component(pb2_reaction, all_molecules, entry, role, role_name) - condition_group: UdmDict = variation.get("CONDITIONS", {}).get( - "CONDITION_GROUP", {} - ) - if condition_group: + conditions: UdmDict = variation.get("CONDITIONS", {}) + # is a direct child of , not . + preparations = [] + for prep in _as_list(conditions.get("PREPARATION")): + text, _ = _text_and_unit(prep) + if text: + preparations.append(text) + if preparations: + pb2_reaction.setup.environment.type = ( + reaction_pb2.ReactionSetup.ReactionEnvironment.CUSTOM + ) + pb2_reaction.setup.environment.details = "; ".join(preparations) + # may repeat ; ORD has only one + # ReactionConditions per Reaction, so later groups overwrite fields set + # by earlier ones rather than being merged or dropped. + for condition_group in _as_list(conditions.get("CONDITION_GROUP")): _add_conditions(pb2_reaction, condition_group) comment = variation.get("COMMENT") @@ -217,7 +306,14 @@ def _add_variation( for udm_product in _as_list(variation.get("PRODUCT")): _add_product(pb2_reaction, all_molecules, udm_product) - _add_provenance(pb2_reaction, reaction, variation, udm_reactions) + _add_provenance( + pb2_reaction, + reaction, + variation, + udm_reactions, + submitter_username, + submitter_email, + ) pb2_reaction.provenance.is_mined = False @@ -236,11 +332,14 @@ def _add_component( _set_molecule_identifier(component, all_molecules, mol_id, role_label) component.reaction_role = role if "AMOUNT" in entry: - # UDM's parsed is a bare number with no unit attribute visible - # in the source data available at the time of writing; grams are - # assumed. Revisit once real UDM sample data confirms the unit. - component.amount.mass.value = float(entry["AMOUNT"]) - component.amount.mass.units = reaction_pb2.Mass.GRAM + # UDM's (molType) is molar, defaulting to "mol"; it is not a + # mass. See _MOLES_UNITS. + text, unit = _text_and_unit(entry["AMOUNT"]) + if text: + component.amount.moles.value = float(text) + component.amount.moles.units = _MOLES_UNITS.get( + unit or "mol", reaction_pb2.Moles.UNSPECIFIED + ) def _set_molecule_identifier( @@ -272,27 +371,30 @@ def _set_molecule_identifier( def _add_conditions( pb2_reaction: reaction_pb2.Reaction, condition_group: UdmDict ) -> None: - """Populates ReactionConditions/ReactionSetup from a UDM CONDITION_GROUP.""" + """Populates ReactionConditions from a UDM CONDITION_GROUP.""" temperature = condition_group.get("TEMPERATURE", {}) if "exact" in temperature: - # No unit is present in the source data available at the time of - # writing, so Celsius is assumed. pb2_reaction.conditions.temperature.setpoint.value = float(temperature["exact"]) - pb2_reaction.conditions.temperature.setpoint.units = ( - reaction_pb2.Temperature.CELSIUS + pb2_reaction.conditions.temperature.setpoint.units = _TEMPERATURE_UNITS.get( + temperature.get("@unit", "degC"), reaction_pb2.Temperature.UNSPECIFIED ) pressure = condition_group.get("PRESSURE", {}) if "exact" in pressure: - # Same caveat as temperature: unit assumed, not present in the source data. pb2_reaction.conditions.pressure.setpoint.value = float(pressure["exact"]) - pb2_reaction.conditions.pressure.setpoint.units = ( - reaction_pb2.Pressure.ATMOSPHERE + pb2_reaction.conditions.pressure.setpoint.units = _PRESSURE_UNITS.get( + pressure.get("@unit", "torr"), reaction_pb2.Pressure.UNSPECIFIED ) - if "STIRRING" in condition_group: + # UDM's STIRRING is a numeric rate (stirringRange, @unit defaulting to + # "rpm"), not free text; ORD's StirringConditions.rate.rpm is the closest + # match. Units other than rpm can't be represented and are dropped. + stirring = condition_group.get("STIRRING", {}) + if "exact" in stirring and stirring.get("@unit", "rpm") == "rpm": + rpm = round(float(stirring["exact"])) pb2_reaction.conditions.stirring.type = reaction_pb2.StirringConditions.CUSTOM - pb2_reaction.conditions.stirring.details = condition_group["STIRRING"] + pb2_reaction.conditions.stirring.details = f"{rpm} rpm" + pb2_reaction.conditions.stirring.rate.rpm = rpm if "REFLUX" in condition_group: pb2_reaction.conditions.reflux = _parse_bool(condition_group["REFLUX"]) @@ -301,12 +403,6 @@ def _add_conditions( if "exact" in ph: pb2_reaction.conditions.ph = float(ph["exact"]) - if "PREPARATION" in condition_group: - pb2_reaction.setup.environment.type = ( - reaction_pb2.ReactionSetup.ReactionEnvironment.CUSTOM - ) - pb2_reaction.setup.environment.details = condition_group["PREPARATION"] - def _parse_bool(value: str) -> bool: """Parses a UDM boolean-ish string (e.g. "true"/"false"/"1"/"0").""" @@ -338,23 +434,45 @@ def _add_provenance( reaction: UdmDict, variation: UdmDict, udm_reactions: UdmDict, + submitter_username: str, + submitter_email: str | None, ) -> None: """Populates ReactionProvenance from reaction- and variation-level UDM fields.""" + # record_created/record_modified describe who is creating this ORD + # record -- i.e. whoever is running this conversion -- not the original + # UDM scientist, who is recorded separately below as the experimenter. + pb2_reaction.provenance.record_created.person.username = submitter_username + if submitter_email: + pb2_reaction.provenance.record_created.person.email = submitter_email + legal: UdmDict = udm_reactions.get("LEGAL", {}) producer = legal.get("PRODUCER") if producer: + # Fallback organization; overridden below if the scientist has their + # own affiliation, since PRODUCER is the data vendor (e.g. "REAXYS"), + # not necessarily the scientist's research institution. pb2_reaction.provenance.experimenter.organization = producer - pb2_reaction.provenance.record_created.person.organization = producer - scientist = variation.get("SCIENTIST") - if scientist: + # SCIENTIST is UDM's authorDetails type (NAME/EMAIL/PHONE/ORGANISATION), + # not a plain string, and may repeat; the first is taken as the + # experimenter of record. + scientists = _as_list(variation.get("SCIENTIST")) + scientist = scientists[0] if scientists else None + if isinstance(scientist, dict): + if scientist.get("NAME"): + pb2_reaction.provenance.experimenter.name = scientist["NAME"] + if scientist.get("EMAIL"): + pb2_reaction.provenance.experimenter.email = scientist["EMAIL"] + organisation = scientist.get("ORGANISATION") + if isinstance(organisation, dict): + if organisation.get("NAME"): + pb2_reaction.provenance.experimenter.organization = organisation["NAME"] + if organisation.get("ADDRESS"): + pb2_reaction.provenance.city = organisation["ADDRESS"] + elif scientist: + # Defensive fallback in case a non-conformant UDM file has a bare + # string SCIENTIST rather than the schema's authorDetails structure. pb2_reaction.provenance.experimenter.name = scientist - pb2_reaction.provenance.record_created.person.name = scientist - - if "ORGANISATIONS" in reaction: - pb2_reaction.provenance.city = reaction["ORGANISATIONS"][0]["ORGANISATION"][ - "ADDRESS" - ] doi = legal.get("DOI") if doi and "CITATIONS" not in reaction: @@ -386,10 +504,9 @@ def _add_provenance( if modification_date: event = pb2_reaction.provenance.record_modified.add() event.time.value = modification_date - if scientist: - event.person.name = scientist - if producer: - event.person.organization = producer + event.person.username = submitter_username + if submitter_email: + event.person.email = submitter_email # Converts XML tree into Python dictionary. diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index d8ab317a..c3f05ae7 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -13,6 +13,7 @@ # limitations under the License. """Tests for ord_schema.scripts.convert_udm_to_ord.""" +import getpass import pathlib import pytest @@ -28,10 +29,23 @@ """ -_ONE_VARIATION_REACTION = """ +# SCIENTIST is UDM's authorDetails type (NAME/EMAIL/PHONE/ORGANISATION), not a +# bare string -- see udm_6_0_0.xsd. +_SCIENTIST = """ + + Test Scientist + scientist@example.com + + Scientist Org +
123 Lab St
+ + +""" + +_ONE_VARIATION_REACTION = f""" - Test Scientist + {_SCIENTIST} 2020-01-01 Reactant A @@ -42,8 +56,11 @@ 85.0 + Stirred under nitrogen. - 25.0 + 298.0 + 1.5 + 500 A test reaction @@ -57,7 +74,7 @@ def _udm_xml(reactions_xml: str) -> str: return f""" Test Dataset - Test Org + Data Vendor 10.0000/test-doi {_MOLECULES} @@ -77,10 +94,14 @@ def _write(tmp_path: pathlib.Path, filename: str, content: str) -> str: def test_simple(tmp_path): input_filename = _write(tmp_path, "input.xml", _udm_xml(_ONE_VARIATION_REACTION)) output_filename = str(tmp_path / "output.pbtxt") - # UDM's SCIENTIST field carries only a name, no email, so a person's - # email -- required by ORD validation -- can never be filled in from UDM - # source data alone. Validation is disabled here for that known reason. - argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + argv = [ + "--input", + input_filename, + "--output", + output_filename, + "--email", + "submitter@example.com", + ] convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) @@ -89,18 +110,37 @@ def test_simple(tmp_path): assert len(dataset.reactions) == 1 reaction = dataset.reactions[0] + # The experimenter is the original UDM scientist who ran the reaction, + # with their own organization/email/address taking precedence over the + # dataset-level PRODUCER (a data vendor, not a research institution). assert reaction.provenance.experimenter.name == "Test Scientist" + assert reaction.provenance.experimenter.email == "scientist@example.com" + assert reaction.provenance.experimenter.organization == "Scientist Org" + assert reaction.provenance.city == "123 Lab St" + + # record_created is whoever ran this conversion, not the UDM scientist. + assert reaction.provenance.record_created.person.username == getpass.getuser() + assert reaction.provenance.record_created.person.email == "submitter@example.com" + assert reaction.provenance.record_created.person.name != "Test Scientist" assert reaction.provenance.record_created.time.value == "2020-01-01" - assert reaction.conditions.temperature.setpoint.value == 25.0 + + assert reaction.conditions.temperature.setpoint.value == 298.0 assert ( reaction.conditions.temperature.setpoint.units - == reaction.conditions.temperature.setpoint.CELSIUS + == reaction.conditions.temperature.setpoint.KELVIN + ) + assert reaction.conditions.pressure.setpoint.value == 1.5 + assert ( + reaction.conditions.pressure.setpoint.units + == reaction.conditions.pressure.setpoint.BAR ) + assert reaction.conditions.stirring.rate.rpm == 500.0 + assert reaction.setup.environment.details == "Stirred under nitrogen." assert reaction.observations[0].comment == "A test reaction" [component] = reaction.inputs["m1"].components - assert component.amount.mass.value == 1.0 - assert component.amount.mass.units == component.amount.mass.GRAM + assert component.amount.moles.value == 1.0 + assert component.amount.moles.units == component.amount.moles.MOLE [outcome] = reaction.outcomes [product] = outcome.products @@ -109,6 +149,49 @@ def test_simple(tmp_path): assert measurement.float_value.value == 85.0 +def test_scientist_organization_falls_back_to_producer(tmp_path): + """When the scientist has no inline ORGANISATION, PRODUCER fills in.""" + reaction_xml = """ + + + No Org Scientist + + Reactant A + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + assert reaction.provenance.experimenter.name == "No Org Scientist" + assert reaction.provenance.experimenter.organization == "Data Vendor" + + +def test_record_created_defaults_to_submitter_without_email(tmp_path): + """Without --email, record_created still identifies the submitter (by OS + username) rather than the UDM scientist; the dataset just won't pass + full validation without an explicit --email, since record_created still + needs one even though the scientist's own email (if UDM supplies it) + goes to experimenter, not record_created. + """ + input_filename = _write(tmp_path, "input.xml", _udm_xml(_ONE_VARIATION_REACTION)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + assert reaction.provenance.record_created.person.username == getpass.getuser() + assert not reaction.provenance.record_created.person.email + assert reaction.provenance.experimenter.name == "Test Scientist" + assert reaction.provenance.experimenter.email == "scientist@example.com" + + def test_multiple_variations_become_separate_reactions(tmp_path): """Regression test: each must produce its own Reaction. @@ -119,7 +202,7 @@ def test_multiple_variations_become_separate_reactions(tmp_path): reaction_xml = """ - Scientist One + Scientist One Reactant A 1.0 @@ -129,7 +212,7 @@ def test_multiple_variations_become_separate_reactions(tmp_path): - Scientist Two + Scientist Two Reactant A 2.0 @@ -150,7 +233,7 @@ def test_multiple_variations_become_separate_reactions(tmp_path): scientists = {r.provenance.experimenter.name for r in dataset.reactions} assert scientists == {"Scientist One", "Scientist Two"} amounts = { - r.inputs["m1"].components[0].amount.mass.value for r in dataset.reactions + r.inputs["m1"].components[0].amount.moles.value for r in dataset.reactions } assert amounts == {1.0, 2.0} @@ -174,6 +257,30 @@ def test_reaction_without_variation_still_emits_one_reaction(tmp_path): assert dataset.reactions[0].identifiers[0].value == "CC.O>>CCO" +def test_single_reactant_id_is_not_split_into_characters(tmp_path): + """Regression test: a lone must not be iterated as a string. + + UDM's REACTANT_ID/PRODUCT_ID are maxOccurs="unbounded"; etree_to_dict + collapses a single occurrence to a bare string rather than a one-element + list, so naive iteration over it would previously produce one bogus + identifier per character. + """ + reaction_xml = """ + + abc123 + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + [component] = dataset.reactions[0].inputs["REACTANT_IDS"].components + assert len(component.identifiers) == 1 + assert component.identifiers[0].value == "abc123" + + def test_unknown_molecule_reference_does_not_crash(tmp_path): reaction_xml = """ From 423fc109b2b83d1426f2422b7e6280eccc77955f Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 15:39:57 +0100 Subject: [PATCH 04/15] Add --include-udm-xml to embed raw UDM source in reaction_metadata 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 element this Reaction came from. Identical across all Reactions produced from the same 's multiple s. - udm_parent_xml: the UDM document-level context (UDM_VERSION/LEGAL/ ORGANISATIONS/CITATIONS) shared by every reaction in the file. 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 elements in parallel with the etree_to_dict'd data (both traverse children in document order, so index i refers to the same 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 --- ord_schema/scripts/convert_udm_to_ord.py | 83 +++++++++++++++++-- ord_schema/scripts/convert_udm_to_ord_test.py | 60 ++++++++++++++ 2 files changed, 138 insertions(+), 5 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 6f82382b..3579a46e 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -107,6 +107,20 @@ def parse_args(argv: list[str] | None = None) -> argparse.Namespace: "the experimenter -- is who is creating this ORD record." ), ) + parser.add_argument( + "--include-udm-xml", + action="store_true", + help=( + "If set, embed the raw UDM source XML for each reaction (and the " + "document-level UDM_VERSION/LEGAL/ORGANISATIONS/CITATIONS context " + "shared by every reaction) in provenance.reaction_metadata. Useful " + "for provenance and for humans spot-checking the converter's " + "output against the original UDM. Off by default: the source XML " + "may carry a different license than the converted dataset (see " + "the CC-BY-SA warning above), so only enable this for UDM sources " + "you have the rights to redistribute." + ), + ) parser.add_argument( "--no-validate", action="store_true", @@ -164,9 +178,26 @@ def main(args: argparse.Namespace) -> None: # (recorded separately as the experimenter). submitter_username = getpass.getuser() + parent_xml = ( + _document_context_xml(udm_tree.getroot()) if args.include_udm_xml else None + ) + # Parallel to _as_list(...REACTION) below: etree_to_dict and findall() both + # walk children in document order, so index i corresponds to the same + # in both. + reaction_elements: list[ET.Element] = [] + if args.include_udm_xml: + reactions_element = udm_tree.getroot().find("REACTIONS") + assert reactions_element is not None # Already validated above. + reaction_elements = reactions_element.findall("REACTION") + pb2_reactions = [] - for reaction in _as_list(udm_reactions["REACTIONS"]["REACTION"]): - base_reaction = _build_base_reaction(reaction) + for i, reaction in enumerate(_as_list(udm_reactions["REACTIONS"]["REACTION"])): + reaction_xml = ( + ET.tostring(reaction_elements[i], encoding="unicode") + if args.include_udm_xml + else None + ) + base_reaction = _build_base_reaction(reaction, reaction_xml, parent_xml) # Each variation becomes its own Reaction, seeded with the # reaction-level identifiers built above. A reaction with no # elements still gets a single Reaction, since it has no @@ -226,15 +257,57 @@ def _text_and_unit(value: Any) -> tuple[str | None, str | None]: return value, None -def _build_base_reaction(reaction: UdmDict) -> reaction_pb2.Reaction: +def _document_context_xml(root: ET.Element) -> str: + """Serializes the UDM document-level context shared by every reaction. + + This is everything under the root except (reaction data, + handled per-reaction) and (a lookup table already captured via + each Compound's identifiers, and often large). + """ + context = ET.Element(root.tag, root.attrib) + for child in root: + if child.tag not in ("REACTIONS", "MOLECULES"): + context.append(child) + return ET.tostring(context, encoding="unicode") + + +def _set_xml_metadata( + reaction_metadata: Any, key: str, xml_text: str, description: str +) -> None: + """Stores raw XML text as a Data entry in a reaction_metadata map.""" + data = reaction_metadata[key] + data.string_value = xml_text + data.format = "xml" + data.description = description + + +def _build_base_reaction( + reaction: UdmDict, reaction_xml: str | None, parent_xml: str | None +) -> reaction_pb2.Reaction: """Builds the reaction-level data shared by every variation of `reaction`. Covers the UDM fields that live directly on rather than on a - specific : REACTANT_ID/PRODUCT_ID placeholders and RXNSTRUCTURE - identifiers. + specific : REACTANT_ID/PRODUCT_ID placeholders, RXNSTRUCTURE + identifiers, and (if requested via --include-udm-xml) the raw source XML. """ base_reaction = reaction_pb2.Reaction() + if reaction_xml: + _set_xml_metadata( + base_reaction.provenance.reaction_metadata, + "udm_reaction_xml", + reaction_xml, + "Raw UDM XML element this Reaction was converted from.", + ) + if parent_xml: + _set_xml_metadata( + base_reaction.provenance.reaction_metadata, + "udm_parent_xml", + parent_xml, + "Raw UDM document-level context (UDM_VERSION/LEGAL/ORGANISATIONS/" + "CITATIONS) shared by every reaction in the source file.", + ) + if "REACTANT_ID" in reaction: molinput = base_reaction.inputs["REACTANT_IDS"] for reactant_id in _as_list(reaction["REACTANT_ID"]): diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index c3f05ae7..5c9b4a86 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -148,6 +148,66 @@ def test_simple(tmp_path): [measurement] = product.measurements assert measurement.float_value.value == 85.0 + # Off by default: no raw UDM XML embedded without --include-udm-xml. + assert "udm_reaction_xml" not in reaction.provenance.reaction_metadata + assert "udm_parent_xml" not in reaction.provenance.reaction_metadata + + +def test_include_udm_xml(tmp_path): + reaction_xml = """ + + + Scientist One + + Reactant A + + + + Scientist Two + + Reactant A + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = [ + "--input", + input_filename, + "--output", + output_filename, + "--no-validate", + "--include-udm-xml", + ] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + assert len(dataset.reactions) == 2 + + for reaction in dataset.reactions: + metadata = reaction.provenance.reaction_metadata + assert metadata["udm_reaction_xml"].format == "xml" + assert "" in metadata["udm_reaction_xml"].string_value + # Both VARIATIONs came from the same , so they share + # identical reaction-level XML, including both scientists. + assert "Scientist One" in metadata["udm_reaction_xml"].string_value + assert "Scientist Two" in metadata["udm_reaction_xml"].string_value + assert metadata["udm_parent_xml"].format == "xml" + assert "Test Dataset" in metadata["udm_parent_xml"].string_value + assert "Data Vendor" in metadata["udm_parent_xml"].string_value + # MOLECULES is deliberately excluded: it's a lookup table, not + # reaction-specific context, and can be large. + assert "MOLECULES" not in metadata["udm_parent_xml"].string_value + + # The two reactions' udm_reaction_xml differ only by REACTION-vs-VARIATION + # scope, not by content -- both variations share one element. + reaction_xmls = { + r.provenance.reaction_metadata["udm_reaction_xml"].string_value + for r in dataset.reactions + } + assert len(reaction_xmls) == 1 + def test_scientist_organization_falls_back_to_producer(tmp_path): """When the scientist has no inline ORGANISATION, PRODUCER fills in.""" From 547fabdbf491394a9969c207128f65d04ae9f6d6 Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 16:03:17 +0100 Subject: [PATCH 05/15] Split multiple CONDITION_GROUPs into separate Reactions, not one merge 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 --- ord_schema/scripts/convert_udm_to_ord.py | 51 +++++++++++-------- ord_schema/scripts/convert_udm_to_ord_test.py | 50 ++++++++++++++++++ 2 files changed, 80 insertions(+), 21 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 3579a46e..408f2806 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -14,9 +14,13 @@ """Builds a Dataset from a set of (UDM) Reactions. Given a UDM file, converts it to a Dataset .pbtxt for ORD. A UDM -may contain multiple elements; each variation is emitted as its -own ORD Reaction, since UDM variations represent distinct experiments run -under the same reaction identifiers. +may contain multiple elements, and a may itself +contain multiple elements; each (variation, condition +group) pair is emitted as its own ORD Reaction, since ORD's Reaction has +only one ReactionConditions and UDM's groups can describe genuinely +different condition sets rather than stages of one experiment. Merging them +into a single ReactionConditions would fabricate a composite that never +existed in the source. UDM (Unified Data Model) is a Pistoia Alliance format; see https://github.com/PistoiaAlliance/UDM for the schema and example data. @@ -203,18 +207,23 @@ def main(args: argparse.Namespace) -> None: # elements still gets a single Reaction, since it has no # variation-specific data to add. for variation in _as_list(reaction.get("VARIATION")) or [{}]: - pb2_reaction = reaction_pb2.Reaction() - pb2_reaction.CopyFrom(base_reaction) - _add_variation( - pb2_reaction, - reaction, - variation, - udm_reactions, - all_molecules, - submitter_username, - args.email, - ) - pb2_reactions.append(pb2_reaction) + conditions: UdmDict = variation.get("CONDITIONS", {}) + # A variation with no still gets one Reaction, + # with ReactionConditions left unset. + for condition_group in _as_list(conditions.get("CONDITION_GROUP")) or [{}]: + pb2_reaction = reaction_pb2.Reaction() + pb2_reaction.CopyFrom(base_reaction) + _add_variation( + pb2_reaction, + reaction, + variation, + udm_reactions, + all_molecules, + submitter_username, + args.email, + condition_group, + ) + pb2_reactions.append(pb2_reaction) dataset = dataset_pb2.Dataset( name=dataset_name, description=dataset_description, reactions=pb2_reactions @@ -348,14 +357,16 @@ def _add_variation( all_molecules: dict[str, UdmDict], submitter_username: str, submitter_email: str | None, + condition_group: UdmDict, ) -> None: - """Populates a Reaction already seeded with base data from one VARIATION.""" + """Populates a Reaction from one (VARIATION, CONDITION_GROUP) pair.""" for role_name, role in _ROLES.items(): for entry in _as_list(variation.get(role_name)): _add_component(pb2_reaction, all_molecules, entry, role, role_name) conditions: UdmDict = variation.get("CONDITIONS", {}) - # is a direct child of , not . + # is a direct child of , not , + # so it applies to every condition group split out of this variation. preparations = [] for prep in _as_list(conditions.get("PREPARATION")): text, _ = _text_and_unit(prep) @@ -366,10 +377,8 @@ def _add_variation( reaction_pb2.ReactionSetup.ReactionEnvironment.CUSTOM ) pb2_reaction.setup.environment.details = "; ".join(preparations) - # may repeat ; ORD has only one - # ReactionConditions per Reaction, so later groups overwrite fields set - # by earlier ones rather than being merged or dropped. - for condition_group in _as_list(conditions.get("CONDITION_GROUP")): + + if condition_group: _add_conditions(pb2_reaction, condition_group) comment = variation.get("COMMENT") diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index 5c9b4a86..1508dfef 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -298,6 +298,56 @@ def test_multiple_variations_become_separate_reactions(tmp_path): assert amounts == {1.0, 2.0} +def test_multiple_condition_groups_become_separate_reactions(tmp_path): + """Regression test: each must produce its own Reaction. + + Merging multiple groups into one ReactionConditions (last field wins per + field) can fabricate a condition set -- e.g. one group's temperature + combined with a different, unrelated group's pressure -- that never + existed in the source. + """ + reaction_xml = """ + + + Test Scientist + + Reactant A + + + + 25.0 + + + 2.0 + + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + assert len(dataset.reactions) == 2 + + # Both reactions share everything except conditions: same scientist, + # same reactant, since they came from the same VARIATION. + scientists = {r.provenance.experimenter.name for r in dataset.reactions} + assert scientists == {"Test Scientist"} + + temperatures = {r.conditions.temperature.setpoint.value for r in dataset.reactions} + pressures = {r.conditions.pressure.setpoint.value for r in dataset.reactions} + assert temperatures == {25.0, 0.0} + assert pressures == {0.0, 2.0} + # Neither reaction has both fields set -- they weren't merged. + for reaction in dataset.reactions: + has_temperature = reaction.conditions.temperature.setpoint.value != 0.0 + has_pressure = reaction.conditions.pressure.setpoint.value != 0.0 + assert has_temperature != has_pressure + + def test_reaction_without_variation_still_emits_one_reaction(tmp_path): reaction_xml = """ From d12b47a53aa8287d10ef1f7b7d5c6cb11e6fd0a3 Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 16:19:17 +0100 Subject: [PATCH 06/15] Don't split CONDITION_GROUPs into separate Reactions either 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 --- ord_schema/scripts/convert_udm_to_ord.py | 75 +++++++++++-------- ord_schema/scripts/convert_udm_to_ord_test.py | 47 ++++++------ 2 files changed, 71 insertions(+), 51 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 408f2806..586a1700 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -14,13 +14,13 @@ """Builds a Dataset from a set of (UDM) Reactions. Given a UDM file, converts it to a Dataset .pbtxt for ORD. A UDM -may contain multiple elements, and a may itself -contain multiple elements; each (variation, condition -group) pair is emitted as its own ORD Reaction, since ORD's Reaction has -only one ReactionConditions and UDM's groups can describe genuinely -different condition sets rather than stages of one experiment. Merging them -into a single ReactionConditions would fabricate a composite that never -existed in the source. +may contain multiple elements; each variation is emitted as its +own ORD Reaction, since UDM variations represent distinct experiments run +under the same reaction identifiers. A may itself contain +multiple elements; ORD's Reaction has only one +ReactionConditions, and UDM does not document precisely enough what +multiple groups within one variation mean to safely merge or split them, so +only the first group is represented structurally (see _add_variation). UDM (Unified Data Model) is a Pistoia Alliance format; see https://github.com/PistoiaAlliance/UDM for the schema and example data. @@ -207,23 +207,18 @@ def main(args: argparse.Namespace) -> None: # elements still gets a single Reaction, since it has no # variation-specific data to add. for variation in _as_list(reaction.get("VARIATION")) or [{}]: - conditions: UdmDict = variation.get("CONDITIONS", {}) - # A variation with no still gets one Reaction, - # with ReactionConditions left unset. - for condition_group in _as_list(conditions.get("CONDITION_GROUP")) or [{}]: - pb2_reaction = reaction_pb2.Reaction() - pb2_reaction.CopyFrom(base_reaction) - _add_variation( - pb2_reaction, - reaction, - variation, - udm_reactions, - all_molecules, - submitter_username, - args.email, - condition_group, - ) - pb2_reactions.append(pb2_reaction) + pb2_reaction = reaction_pb2.Reaction() + pb2_reaction.CopyFrom(base_reaction) + _add_variation( + pb2_reaction, + reaction, + variation, + udm_reactions, + all_molecules, + submitter_username, + args.email, + ) + pb2_reactions.append(pb2_reaction) dataset = dataset_pb2.Dataset( name=dataset_name, description=dataset_description, reactions=pb2_reactions @@ -357,16 +352,14 @@ def _add_variation( all_molecules: dict[str, UdmDict], submitter_username: str, submitter_email: str | None, - condition_group: UdmDict, ) -> None: - """Populates a Reaction from one (VARIATION, CONDITION_GROUP) pair.""" + """Populates a Reaction already seeded with base data from one VARIATION.""" for role_name, role in _ROLES.items(): for entry in _as_list(variation.get(role_name)): _add_component(pb2_reaction, all_molecules, entry, role, role_name) conditions: UdmDict = variation.get("CONDITIONS", {}) - # is a direct child of , not , - # so it applies to every condition group split out of this variation. + # is a direct child of , not . preparations = [] for prep in _as_list(conditions.get("PREPARATION")): text, _ = _text_and_unit(prep) @@ -378,8 +371,30 @@ def _add_variation( ) pb2_reaction.setup.environment.details = "; ".join(preparations) - if condition_group: - _add_conditions(pb2_reaction, condition_group) + # may repeat . Splitting each group into + # its own Reaction (an earlier version of this code did that) is wrong + # too: REACTANT/PRODUCT/YIELD/COMMENT/provenance are variation-level, not + # per-group, so splitting would duplicate a single recorded outcome into + # every group's Reaction, asserting it was independently reproduced under + # each condition set. Since UDM doesn't document what multiple groups + # mean well enough to resolve that safely, only the first group is + # represented structurally; the rest are noted, not merged or dropped + # silently, so at least no fabricated composite or duplicate is created. + condition_groups = _as_list(conditions.get("CONDITION_GROUP")) + if condition_groups: + _add_conditions(pb2_reaction, condition_groups[0]) + if len(condition_groups) > 1: + logger.warning( + "VARIATION has %d CONDITION_GROUPs; only the first is " + "represented in structured fields (see conditions.details).", + len(condition_groups), + ) + pb2_reaction.conditions.details = ( + f"UDM recorded {len(condition_groups)} CONDITION_GROUP entries " + "for this variation; only the first is represented in the " + "structured fields above. Re-run with --include-udm-xml to " + "recover the rest from the source." + ) comment = variation.get("COMMENT") if comment: diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index 1508dfef..fac7a744 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -298,13 +298,19 @@ def test_multiple_variations_become_separate_reactions(tmp_path): assert amounts == {1.0, 2.0} -def test_multiple_condition_groups_become_separate_reactions(tmp_path): - """Regression test: each must produce its own Reaction. - - Merging multiple groups into one ReactionConditions (last field wins per - field) can fabricate a condition set -- e.g. one group's temperature - combined with a different, unrelated group's pressure -- that never - existed in the source. +def test_multiple_condition_groups_use_first_group_only(tmp_path): + """Regression test: multiple s must not be merged, and + must not multiply the Reaction. + + An earlier version of this code merged every group's fields into one + ReactionConditions (fabricating composites, e.g. one group's temperature + plus an unrelated group's pressure, that never existed in the source). A + later version instead split each group into its own Reaction -- also + wrong, since REACTANT/PRODUCT/YIELD/COMMENT/provenance are variation- + level, not per-group, so splitting duplicated a single recorded outcome + across multiple Reactions as if it were independently reproduced under + each condition set. Only the first group is represented structurally; + the existence of the rest is noted, not silently dropped. """ reaction_xml = """ @@ -313,6 +319,10 @@ def test_multiple_condition_groups_become_separate_reactions(tmp_path): Reactant A + + Product A + 85.0 + 25.0 @@ -330,22 +340,17 @@ def test_multiple_condition_groups_become_separate_reactions(tmp_path): convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) - assert len(dataset.reactions) == 2 + assert len(dataset.reactions) == 1 + reaction = dataset.reactions[0] - # Both reactions share everything except conditions: same scientist, - # same reactant, since they came from the same VARIATION. - scientists = {r.provenance.experimenter.name for r in dataset.reactions} - assert scientists == {"Test Scientist"} + # Only the outcome recorded once in the source, not duplicated. + assert len(reaction.outcomes) == 1 + assert reaction.outcomes[0].products[0].measurements[0].float_value.value == 85.0 - temperatures = {r.conditions.temperature.setpoint.value for r in dataset.reactions} - pressures = {r.conditions.pressure.setpoint.value for r in dataset.reactions} - assert temperatures == {25.0, 0.0} - assert pressures == {0.0, 2.0} - # Neither reaction has both fields set -- they weren't merged. - for reaction in dataset.reactions: - has_temperature = reaction.conditions.temperature.setpoint.value != 0.0 - has_pressure = reaction.conditions.pressure.setpoint.value != 0.0 - assert has_temperature != has_pressure + # Only the first group's field is structurally represented. + assert reaction.conditions.temperature.setpoint.value == 25.0 + assert reaction.conditions.pressure.setpoint.value == 0.0 + assert "2 CONDITION_GROUP" in reaction.conditions.details def test_reaction_without_variation_still_emits_one_reaction(tmp_path): From 1f521333922adc3b4d8edfff81097d15439e2b23 Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 16:37:02 +0100 Subject: [PATCH 07/15] Represent multiple CONDITION_GROUPs via conditions_are_dynamic, not a 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 -- 2020 165165 -- 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 -- 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 --- ord_schema/scripts/convert_udm_to_ord.py | 132 +++++++++++++----- ord_schema/scripts/convert_udm_to_ord_test.py | 76 +++++++--- 2 files changed, 156 insertions(+), 52 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 586a1700..72c4fb68 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -17,10 +17,12 @@ may contain multiple elements; each variation is emitted as its own ORD Reaction, since UDM variations represent distinct experiments run under the same reaction identifiers. A may itself contain -multiple elements; ORD's Reaction has only one -ReactionConditions, and UDM does not document precisely enough what -multiple groups within one variation mean to safely merge or split them, so -only the first group is represented structurally (see _add_variation). +multiple elements, which per the UDM ChangeLog model a +single dynamic, multi-stage condition profile (e.g. "20 degC for 23 min, +then 165 degC for 5 min") rather than alternative readings or separate +experiments; these are recorded via ReactionConditions.conditions_are_dynamic +and .details rather than merged into or split across a static single-step +ReactionConditions (see _add_variation). UDM (Unified Data Model) is a Pistoia Alliance format; see https://github.com/PistoiaAlliance/UDM for the schema and example data. @@ -261,6 +263,63 @@ def _text_and_unit(value: Any) -> tuple[str | None, str | None]: return value, None +def _parse_range(value: UdmDict) -> tuple[float, float | None] | None: + """Parses a UDM min/max/exact range into (midpoint, precision). + + UDM's *Range types (temperatureRange, pressureRange, etc.) hold exactly + one of {min, max, min and max, exact} -- see Docs/ChangeLog.md's note on + XML Schema's limited support for "exactly one of" constraints. `exact` + maps to (value, None); a min/max pair maps to their midpoint with + precision = half the spread; a lone min or max maps to that value with + no precision. Returns None if none of min/max/exact is present. + """ + if "exact" in value: + return float(value["exact"]), None + min_v, max_v = value.get("min"), value.get("max") + if min_v is not None and max_v is not None: + min_f, max_f = float(min_v), float(max_v) + return (min_f + max_f) / 2, (max_f - min_f) / 2 + if min_v is not None: + return float(min_v), None + if max_v is not None: + return float(max_v), None + return None + + +def _format_range_text(value: UdmDict, default_unit: str) -> str | None: + """Renders a UDM min/max/exact range as human-readable text, e.g. "20±5 degC".""" + parsed = _parse_range(value) + if parsed is None: + return None + midpoint, precision = parsed + unit = value.get("@unit", default_unit) + if precision: + return f"{midpoint}±{precision} {unit}".strip() + return f"{midpoint} {unit}".strip() + + +def _summarize_condition_group(group: UdmDict, index: int) -> str: + """Renders one CONDITION_GROUP as a human-readable line for .conditions.details.""" + parts = [] + if group.get("PROCESS"): + parts.append(group["PROCESS"]) + for field, default_unit, label in ( + ("TEMPERATURE", "degC", "temperature"), + ("PRESSURE", "torr", "pressure"), + ("TIME", "hr", "time"), + ("STIRRING", "rpm", "stirring"), + ("PH", "", "pH"), + ): + text = _format_range_text(group.get(field, {}), default_unit) + if text: + parts.append(f"{label} {text}") + if "REFLUX" in group: + parts.append(f"reflux={group['REFLUX']}") + if group.get("ATMOSPHERE"): + parts.append(f"atmosphere={group['ATMOSPHERE']}") + return f"Stage {index}: " + ("; ".join(parts) if parts else "(no data)") + + def _document_context_xml(root: ET.Element) -> str: """Serializes the UDM document-level context shared by every reaction. @@ -371,30 +430,25 @@ def _add_variation( ) pb2_reaction.setup.environment.details = "; ".join(preparations) - # may repeat . Splitting each group into - # its own Reaction (an earlier version of this code did that) is wrong - # too: REACTANT/PRODUCT/YIELD/COMMENT/provenance are variation-level, not - # per-group, so splitting would duplicate a single recorded outcome into - # every group's Reaction, asserting it was independently reproduced under - # each condition set. Since UDM doesn't document what multiple groups - # mean well enough to resolve that safely, only the first group is - # represented structurally; the rest are noted, not merged or dropped - # silently, so at least no fabricated composite or duplicate is created. + # may repeat . Per Docs/ChangeLog.md's + # CONDITIONS section (Pistoia Alliance UDM repo), multiple groups model a + # single dynamic, multi-stage condition profile -- e.g. "20 degC for 23 + # min, then 165 degC for 5 min, then held 6 more min" -- not alternative + # readings or separate experiments. Merging them into one static + # ReactionConditions or splitting each into its own Reaction would both + # fabricate data that isn't in the source. ORD models exactly this case + # with conditions_are_dynamic + a free-text details field ("e.g., + # multiple stages" per its own doc comment), so the full staged profile + # is recorded as text instead. condition_groups = _as_list(conditions.get("CONDITION_GROUP")) - if condition_groups: + if len(condition_groups) == 1: _add_conditions(pb2_reaction, condition_groups[0]) - if len(condition_groups) > 1: - logger.warning( - "VARIATION has %d CONDITION_GROUPs; only the first is " - "represented in structured fields (see conditions.details).", - len(condition_groups), - ) - pb2_reaction.conditions.details = ( - f"UDM recorded {len(condition_groups)} CONDITION_GROUP entries " - "for this variation; only the first is represented in the " - "structured fields above. Re-run with --include-udm-xml to " - "recover the rest from the source." - ) + elif len(condition_groups) > 1: + pb2_reaction.conditions.conditions_are_dynamic = True + pb2_reaction.conditions.details = "; ".join( + _summarize_condition_group(group, i) + for i, group in enumerate(condition_groups, start=1) + ) comment = variation.get("COMMENT") if comment: @@ -470,15 +524,23 @@ def _add_conditions( ) -> None: """Populates ReactionConditions from a UDM CONDITION_GROUP.""" temperature = condition_group.get("TEMPERATURE", {}) - if "exact" in temperature: - pb2_reaction.conditions.temperature.setpoint.value = float(temperature["exact"]) + parsed = _parse_range(temperature) + if parsed: + value, precision = parsed + pb2_reaction.conditions.temperature.setpoint.value = value + if precision: + pb2_reaction.conditions.temperature.setpoint.precision = precision pb2_reaction.conditions.temperature.setpoint.units = _TEMPERATURE_UNITS.get( temperature.get("@unit", "degC"), reaction_pb2.Temperature.UNSPECIFIED ) pressure = condition_group.get("PRESSURE", {}) - if "exact" in pressure: - pb2_reaction.conditions.pressure.setpoint.value = float(pressure["exact"]) + parsed = _parse_range(pressure) + if parsed: + value, precision = parsed + pb2_reaction.conditions.pressure.setpoint.value = value + if precision: + pb2_reaction.conditions.pressure.setpoint.precision = precision pb2_reaction.conditions.pressure.setpoint.units = _PRESSURE_UNITS.get( pressure.get("@unit", "torr"), reaction_pb2.Pressure.UNSPECIFIED ) @@ -487,8 +549,9 @@ def _add_conditions( # "rpm"), not free text; ORD's StirringConditions.rate.rpm is the closest # match. Units other than rpm can't be represented and are dropped. stirring = condition_group.get("STIRRING", {}) - if "exact" in stirring and stirring.get("@unit", "rpm") == "rpm": - rpm = round(float(stirring["exact"])) + parsed = _parse_range(stirring) + if parsed and stirring.get("@unit", "rpm") == "rpm": + rpm = round(parsed[0]) pb2_reaction.conditions.stirring.type = reaction_pb2.StirringConditions.CUSTOM pb2_reaction.conditions.stirring.details = f"{rpm} rpm" pb2_reaction.conditions.stirring.rate.rpm = rpm @@ -497,8 +560,9 @@ def _add_conditions( pb2_reaction.conditions.reflux = _parse_bool(condition_group["REFLUX"]) ph = condition_group.get("PH", {}) - if "exact" in ph: - pb2_reaction.conditions.ph = float(ph["exact"]) + parsed = _parse_range(ph) + if parsed: + pb2_reaction.conditions.ph = parsed[0] def _parse_bool(value: str) -> bool: diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index fac7a744..6ab02246 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -298,19 +298,21 @@ def test_multiple_variations_become_separate_reactions(tmp_path): assert amounts == {1.0, 2.0} -def test_multiple_condition_groups_use_first_group_only(tmp_path): - """Regression test: multiple s must not be merged, and - must not multiply the Reaction. - - An earlier version of this code merged every group's fields into one - ReactionConditions (fabricating composites, e.g. one group's temperature - plus an unrelated group's pressure, that never existed in the source). A - later version instead split each group into its own Reaction -- also - wrong, since REACTANT/PRODUCT/YIELD/COMMENT/provenance are variation- - level, not per-group, so splitting duplicated a single recorded outcome - across multiple Reactions as if it were independently reproduced under - each condition set. Only the first group is represented structurally; - the existence of the rest is noted, not silently dropped. +def test_multiple_condition_groups_are_recorded_as_dynamic(tmp_path): + """Regression test: multiple s model one dynamic, + multi-stage profile -- see Docs/ChangeLog.md's CONDITIONS section in the + UDM repo -- not alternative readings or separate experiments. + + Two earlier versions of this code got this wrong in opposite ways: one + merged every group's fields into one static ReactionConditions + (fabricating composites, e.g. one group's temperature plus an unrelated + group's pressure, that never existed in the source); another split each + group into its own Reaction, which duplicated the variation-level + REACTANT/PRODUCT/YIELD/COMMENT/provenance into every split, asserting a + single recorded outcome was independently reproduced under each + condition set. Neither merges nor splits: conditions_are_dynamic and a + full-fidelity text summary in .details record the staged profile, per + the fields ORD itself provides for exactly this case. """ reaction_xml = """ @@ -325,10 +327,15 @@ def test_multiple_condition_groups_use_first_group_only(tmp_path): - 25.0 + 2020 + - 2.0 + 165165 + + + + @@ -347,10 +354,43 @@ def test_multiple_condition_groups_use_first_group_only(tmp_path): assert len(reaction.outcomes) == 1 assert reaction.outcomes[0].products[0].measurements[0].float_value.value == 85.0 - # Only the first group's field is structurally represented. + assert reaction.conditions.conditions_are_dynamic + # No single-step snapshot: none of the three stages is "the" temperature. + assert reaction.conditions.temperature.setpoint.value == 0.0 + assert "Stage 1: temperature 20.0 degC; time 23.0 hr" in reaction.conditions.details + assert "Stage 2: temperature 165.0 degC; time 5.0 hr" in reaction.conditions.details + assert "Stage 3: time 6.0 hr" in reaction.conditions.details + + +def test_condition_group_min_max_range_uses_midpoint_and_precision(tmp_path): + """A single with a min/max range (no ) -- the + form UDM's own documentation example uses -- must still populate the + structured setpoint, not just . + """ + reaction_xml = """ + + + + Reactant A + + + + 2030 + + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + assert not reaction.conditions.conditions_are_dynamic assert reaction.conditions.temperature.setpoint.value == 25.0 - assert reaction.conditions.pressure.setpoint.value == 0.0 - assert "2 CONDITION_GROUP" in reaction.conditions.details + assert reaction.conditions.temperature.setpoint.precision == 5.0 def test_reaction_without_variation_still_emits_one_reaction(tmp_path): From 23277bcfbbe0b84455cdf36da2d4c4e84c82d5e8 Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 17:01:24 +0100 Subject: [PATCH 08/15] Cover all CONDITION_GROUP fields, not just the ones ORD has a slot for 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 --- ord_schema/scripts/convert_udm_to_ord.py | 92 ++++++++++++++++--- ord_schema/scripts/convert_udm_to_ord_test.py | 79 ++++++++++++++++ 2 files changed, 159 insertions(+), 12 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 72c4fb68..43870174 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -298,25 +298,85 @@ def _format_range_text(value: UdmDict, default_unit: str) -> str | None: return f"{midpoint} {unit}".strip() -def _summarize_condition_group(group: UdmDict, index: int) -> str: - """Renders one CONDITION_GROUP as a human-readable line for .conditions.details.""" +# CONDITION_GROUP's *Range-typed fields, with the default @unit each takes +# per the XSD when @unit is omitted (udm_6_0_0.xsd / udm_6_0_0_units.xsd). +_CONDITION_GROUP_RANGE_FIELDS: tuple[tuple[str, str, str], ...] = ( + ("TEMPERATURE", "degC", "temperature"), + ("PRESSURE", "torr", "pressure"), + ("TIME", "hr", "time"), + ("STIRRING", "rpm", "stirring"), + ("PH", "", "pH"), + ("REACTION_MOLARITY", "mol/L", "reaction molarity"), + ("BUFFER_CONCENTRATION", "mol/L", "buffer concentration"), + ("TOTAL_VOLUME", "L", "total volume"), +) + +# CONDITION_GROUP's Id-Ref-typed fields (references to other entities). +_CONDITION_GROUP_ID_REF_FIELDS: tuple[tuple[str, str], ...] = ( + ("REACTANT_ID", "reactants"), + ("REAGENT_ID", "reagents"), + ("CATALYST_ID", "catalysts"), + ("SOLVENT_ID", "solvents"), +) + +# _add_conditions already represents these fields structurally for a single +# CONDITION_GROUP; _condition_group_fields excludes them there so they +# aren't duplicated in .conditions.details. +_STRUCTURED_CONDITION_FIELDS = frozenset( + {"TEMPERATURE", "PRESSURE", "STIRRING", "REFLUX", "PH"} +) + + +def _condition_group_fields( + group: UdmDict, exclude: frozenset[str] = frozenset() +) -> list[str]: + """Renders every known CONDITION_GROUP field as human-readable text fragments. + + Covers the whole CONDITION_GROUP element (udm_6_0_0.xsd), not just the + fields ORD has a matching structured field for, so nothing is silently + dropped: `_add_conditions` (single group) and `_summarize_condition_group` + (multiple, dynamic groups) both build on this rather than each picking + their own subset. + """ parts = [] - if group.get("PROCESS"): + if "PROCESS" not in exclude and group.get("PROCESS"): parts.append(group["PROCESS"]) - for field, default_unit, label in ( - ("TEMPERATURE", "degC", "temperature"), - ("PRESSURE", "torr", "pressure"), - ("TIME", "hr", "time"), - ("STIRRING", "rpm", "stirring"), - ("PH", "", "pH"), - ): + for id_field, label in _CONDITION_GROUP_ID_REF_FIELDS: + if id_field in exclude: + continue + ids = _as_list(group.get(id_field)) + if ids: + parts.append(f"{label}={','.join(ids)}") + for field, default_unit, label in _CONDITION_GROUP_RANGE_FIELDS: + if field in exclude: + continue text = _format_range_text(group.get(field, {}), default_unit) if text: parts.append(f"{label} {text}") - if "REFLUX" in group: + if "REFLUX" not in exclude and "REFLUX" in group: parts.append(f"reflux={group['REFLUX']}") - if group.get("ATMOSPHERE"): + if "BUFFER_TYPE" not in exclude and group.get("BUFFER_TYPE"): + parts.append(f"buffer={group['BUFFER_TYPE']}") + if "ATMOSPHERE" not in exclude and group.get("ATMOSPHERE"): parts.append(f"atmosphere={group['ATMOSPHERE']}") + if "PREPARATION" not in exclude: + preparations = [] + for prep in _as_list(group.get("PREPARATION")): + text, _ = _text_and_unit(prep) + if text: + preparations.append(text) + if preparations: + parts.append(f"preparation={'; '.join(preparations)}") + if "SECTION" not in exclude and "SECTION" in group: + # SECTION is UDM's generic, schema-free extension point; its content + # isn't renderable as text, but its presence is still noted. + parts.append("custom SECTION data present (see --include-udm-xml)") + return parts + + +def _summarize_condition_group(group: UdmDict, index: int) -> str: + """Renders one CONDITION_GROUP as a human-readable line for .conditions.details.""" + parts = _condition_group_fields(group) return f"Stage {index}: " + ("; ".join(parts) if parts else "(no data)") @@ -443,6 +503,14 @@ def _add_variation( condition_groups = _as_list(conditions.get("CONDITION_GROUP")) if len(condition_groups) == 1: _add_conditions(pb2_reaction, condition_groups[0]) + # Fields _add_conditions doesn't have a structured place for (e.g. + # BUFFER_TYPE, REACTION_MOLARITY, agent ID references) still get + # recorded as text rather than silently dropped. + extra = _condition_group_fields( + condition_groups[0], exclude=_STRUCTURED_CONDITION_FIELDS + ) + if extra: + pb2_reaction.conditions.details = "; ".join(extra) elif len(condition_groups) > 1: pb2_reaction.conditions.conditions_are_dynamic = True pb2_reaction.conditions.details = "; ".join( diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index 6ab02246..b2c43e09 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -393,6 +393,85 @@ def test_condition_group_min_max_range_uses_midpoint_and_precision(tmp_path): assert reaction.conditions.temperature.setpoint.precision == 5.0 +def test_condition_group_buffer_and_agent_fields_are_not_dropped(tmp_path): + """Regression test: CONDITION_GROUP fields ORD has no structured field for + (BUFFER_TYPE, BUFFER_CONCENTRATION, REACTION_MOLARITY, TOTAL_VOLUME, + REAGENT_ID/...) must still show up somewhere, not vanish silently just + because they aren't temperature/pressure/stirring/reflux/pH. + """ + reaction_xml = """ + + + + Reactant A + + + + 25.0 + r1 + phosphate + 0.1 + 0.5 + 10 + + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + # Still structurally represented, same as before. + assert reaction.conditions.temperature.setpoint.value == 25.0 + # Everything else lands in details rather than disappearing. + details = reaction.conditions.details + assert "reagents=r1" in details + assert "buffer=phosphate" in details + assert "buffer concentration 0.1 mol/L" in details + assert "reaction molarity 0.5 mol/L" in details + assert "total volume 10.0 L" in details + # Not duplicated: temperature is structural only, not also in details. + assert "temperature" not in details + + +def test_dynamic_condition_groups_include_buffer_and_agent_fields(tmp_path): + """Same as above, but for the multi-group (conditions_are_dynamic) path, + where every field -- including temperature/pressure/stirring/pH -- has + to come from the text summary, since nothing is structurally set there. + """ + reaction_xml = """ + + + + Reactant A + + + + 20.0 + phosphate + + + 40.0 + + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + assert reaction.conditions.conditions_are_dynamic + assert "buffer=phosphate" in reaction.conditions.details + + def test_reaction_without_variation_still_emits_one_reaction(tmp_path): reaction_xml = """ From 36037b97cc40c5900685ffe57f45a9506c95dabe Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 17:12:10 +0100 Subject: [PATCH 09/15] Surface TEMPERATURE's ramp rate, which has no ORD field at all Greptile flagged that _parse_range only reads exact/min/max, so a temperatureRange's optional 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 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 (1005), 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 --- ord_schema/scripts/convert_udm_to_ord.py | 33 ++++++++++++++--- ord_schema/scripts/convert_udm_to_ord_test.py | 37 +++++++++++++++++++ 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 43870174..327ced40 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -298,6 +298,23 @@ def _format_range_text(value: UdmDict, default_unit: str) -> str | None: return f"{midpoint} {unit}".strip() +def _format_incr(value: UdmDict) -> str | None: + """Renders a *Range's optional ramp-rate child as text, if present. + + Only temperatureRange (and percentageRange, unused by CONDITION_GROUP) + define in the XSD; every other range type simply won't have it, + so this is a no-op for them. ORD has no ramp-rate field on Temperature, + so incr never has a structured home -- it must always be surfaced as + text, even when the range's central value is captured structurally. + """ + if "incr" not in value: + return None + text, unit = _text_and_unit(value["incr"]) + if not text: + return None + return f"{text} {unit}".strip() if unit else text + + # CONDITION_GROUP's *Range-typed fields, with the default @unit each takes # per the XSD when @unit is omitted (udm_6_0_0.xsd / udm_6_0_0_units.xsd). _CONDITION_GROUP_RANGE_FIELDS: tuple[tuple[str, str, str], ...] = ( @@ -348,11 +365,17 @@ def _condition_group_fields( if ids: parts.append(f"{label}={','.join(ids)}") for field, default_unit, label in _CONDITION_GROUP_RANGE_FIELDS: - if field in exclude: - continue - text = _format_range_text(group.get(field, {}), default_unit) - if text: - parts.append(f"{label} {text}") + field_value = group.get(field, {}) + if field not in exclude: + text = _format_range_text(field_value, default_unit) + if text: + parts.append(f"{label} {text}") + # incr (ramp rate) has no ORD equivalent regardless of whether the + # central value above is captured structurally, so it's always + # checked, not just when `field` itself isn't excluded. + incr = _format_incr(field_value) + if incr: + parts.append(f"{label} ramp={incr}") if "REFLUX" not in exclude and "REFLUX" in group: parts.append(f"reflux={group['REFLUX']}") if "BUFFER_TYPE" not in exclude and group.get("BUFFER_TYPE"): diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index b2c43e09..032105ac 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -472,6 +472,43 @@ def test_dynamic_condition_groups_include_buffer_and_agent_fields(tmp_path): assert "buffer=phosphate" in reaction.conditions.details +def test_temperature_ramp_incr_is_not_dropped(tmp_path): + """Regression test: 's optional ramp-rate child has + no ORD field to live in, so it must surface as text even in the + single-group case where the central value *is* captured structurally + (Temperature.setpoint) -- excluding all of TEMPERATURE from the "extra" + text pass would silently drop incr along with it. + """ + reaction_xml = """ + + + + Reactant A + + + + + 100 + 5 + + + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + # Central value still structural. + assert reaction.conditions.temperature.setpoint.value == 100.0 + # Ramp rate, which has no structured home, surfaces as text instead. + assert "temperature ramp=5 deg_C/hour" in reaction.conditions.details + + def test_reaction_without_variation_still_emits_one_reaction(tmp_path): reaction_xml = """ From dc627005d66d9662c31b511619421237fa281403 Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 17:21:52 +0100 Subject: [PATCH 10/15] Stop assuming STIRRING is always captured structurally 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 --- ord_schema/scripts/convert_udm_to_ord.py | 45 ++++++++++++------- ord_schema/scripts/convert_udm_to_ord_test.py | 36 +++++++++++++++ 2 files changed, 64 insertions(+), 17 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 327ced40..e9fa3861 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -336,13 +336,6 @@ def _format_incr(value: UdmDict) -> str | None: ("SOLVENT_ID", "solvents"), ) -# _add_conditions already represents these fields structurally for a single -# CONDITION_GROUP; _condition_group_fields excludes them there so they -# aren't duplicated in .conditions.details. -_STRUCTURED_CONDITION_FIELDS = frozenset( - {"TEMPERATURE", "PRESSURE", "STIRRING", "REFLUX", "PH"} -) - def _condition_group_fields( group: UdmDict, exclude: frozenset[str] = frozenset() @@ -525,13 +518,13 @@ def _add_variation( # is recorded as text instead. condition_groups = _as_list(conditions.get("CONDITION_GROUP")) if len(condition_groups) == 1: - _add_conditions(pb2_reaction, condition_groups[0]) - # Fields _add_conditions doesn't have a structured place for (e.g. - # BUFFER_TYPE, REACTION_MOLARITY, agent ID references) still get - # recorded as text rather than silently dropped. - extra = _condition_group_fields( - condition_groups[0], exclude=_STRUCTURED_CONDITION_FIELDS - ) + captured = _add_conditions(pb2_reaction, condition_groups[0]) + # Whatever _add_conditions didn't actually manage to capture + # structurally -- either because ORD has no field for it (e.g. + # BUFFER_TYPE) or because this particular value couldn't be, despite + # the field type usually being structural (e.g. STIRRING in a unit + # other than rpm) -- still gets recorded as text, not dropped. + extra = _condition_group_fields(condition_groups[0], exclude=captured) if extra: pb2_reaction.conditions.details = "; ".join(extra) elif len(condition_groups) > 1: @@ -612,8 +605,17 @@ def _set_molecule_identifier( def _add_conditions( pb2_reaction: reaction_pb2.Reaction, condition_group: UdmDict -) -> None: - """Populates ReactionConditions from a UDM CONDITION_GROUP.""" +) -> frozenset[str]: + """Populates ReactionConditions from a UDM CONDITION_GROUP. + + Returns the CONDITION_GROUP field names actually captured structurally, + so the caller knows what's safe to omit from a free-text fallback (and, + just as importantly, what wasn't captured despite being present and + still needs to be surfaced as text -- e.g. STIRRING in a unit other than + rpm, which ORD's StirringConditions.rate.rpm can't represent). + """ + captured = set() + temperature = condition_group.get("TEMPERATURE", {}) parsed = _parse_range(temperature) if parsed: @@ -624,6 +626,7 @@ def _add_conditions( pb2_reaction.conditions.temperature.setpoint.units = _TEMPERATURE_UNITS.get( temperature.get("@unit", "degC"), reaction_pb2.Temperature.UNSPECIFIED ) + captured.add("TEMPERATURE") pressure = condition_group.get("PRESSURE", {}) parsed = _parse_range(pressure) @@ -635,10 +638,13 @@ def _add_conditions( pb2_reaction.conditions.pressure.setpoint.units = _PRESSURE_UNITS.get( pressure.get("@unit", "torr"), reaction_pb2.Pressure.UNSPECIFIED ) + captured.add("PRESSURE") # UDM's STIRRING is a numeric rate (stirringRange, @unit defaulting to # "rpm"), not free text; ORD's StirringConditions.rate.rpm is the closest - # match. Units other than rpm can't be represented and are dropped. + # match, but only when the unit actually is rpm -- stirringRange's @unit + # is unrestricted xs:string in the XSD, not an enum, so other units are + # possible and can't be represented structurally. stirring = condition_group.get("STIRRING", {}) parsed = _parse_range(stirring) if parsed and stirring.get("@unit", "rpm") == "rpm": @@ -646,14 +652,19 @@ def _add_conditions( pb2_reaction.conditions.stirring.type = reaction_pb2.StirringConditions.CUSTOM pb2_reaction.conditions.stirring.details = f"{rpm} rpm" pb2_reaction.conditions.stirring.rate.rpm = rpm + captured.add("STIRRING") if "REFLUX" in condition_group: pb2_reaction.conditions.reflux = _parse_bool(condition_group["REFLUX"]) + captured.add("REFLUX") ph = condition_group.get("PH", {}) parsed = _parse_range(ph) if parsed: pb2_reaction.conditions.ph = parsed[0] + captured.add("PH") + + return frozenset(captured) def _parse_bool(value: str) -> bool: diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index 032105ac..220497cd 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -509,6 +509,42 @@ def test_temperature_ramp_incr_is_not_dropped(tmp_path): assert "temperature ramp=5 deg_C/hour" in reaction.conditions.details +def test_non_rpm_stirring_is_not_dropped(tmp_path): + """Regression test: STIRRING in a unit other than rpm has no structured + home (StirringConditions.rate.rpm is rpm-specific), so it must surface + as text -- unlike rpm-unit stirring, which _add_conditions does capture + structurally, this one isn't actually captured despite STIRRING usually + being a "structural" field, so it can't be blanket-excluded from the + text pass the way BUFFER_TYPE etc. are (see _add_conditions' return + value, which _add_variation checks rather than assuming). + """ + reaction_xml = """ + + + + Reactant A + + + + 50 + + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + # Not structurally represented: rate.rpm can't hold a Hz value. + assert reaction.conditions.stirring.rate.rpm == 0 + # But not silently dropped either. + assert "stirring 50.0 Hz" in reaction.conditions.details + + def test_reaction_without_variation_still_emits_one_reaction(tmp_path): reaction_xml = """ From 3f6f91d6802f317bcf7d419bc8296c99a98f3fbf Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 18:02:50 +0100 Subject: [PATCH 11/15] Fix YIELD proto field, YIELD range support, and reaction-level CITATIONS 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 / 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 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 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 --- ord_schema/scripts/convert_udm_to_ord.py | 27 ++++-- ord_schema/scripts/convert_udm_to_ord_test.py | 86 ++++++++++++++++++- 2 files changed, 102 insertions(+), 11 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index e9fa3861..cb215175 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -53,10 +53,11 @@ "SOLVENT": reaction_pb2.ReactionRole.SOLVENT, } -# UDM RXNSTRUCTURE @format values that map onto an ORD ReactionIdentifierType. -# Anything not listed here (including UDM's default, unlabeled format) falls -# back to UNSPECIFIED; that reaction identifier is then dropped rather than -# emitted with an invalid type (see _build_base_reaction). +# UDM RXNSTRUCTURE (a child element, not an XML attribute -- see +# _build_base_reaction's structure.get("format", ...)) values that map onto +# an ORD ReactionIdentifierType. Anything not listed here (including UDM's +# default, unlabeled format) falls back to UNSPECIFIED; that reaction +# identifier is then dropped rather than emitted with an invalid type. _RXNSTRUCTURE_FORMATS: dict[str, tuple[int, str]] = { "cdxml": (reaction_pb2.ReactionIdentifier.CUSTOM, "cdxml"), "rinchi": (reaction_pb2.ReactionIdentifier.RINCHI, ""), @@ -685,11 +686,16 @@ def _add_product( _set_molecule_identifier( component, all_molecules, molecule.get("@MOL_ID"), "PRODUCT" ) - yield_ = udm_product.get("YIELD", {}) - if "exact" in yield_: + parsed = _parse_range(udm_product.get("YIELD", {})) + if parsed: + value, precision = parsed measurement = component.measurements.add() measurement.type = reaction_pb2.ProductMeasurement.ProductMeasurementType.YIELD - measurement.float_value.value = float(yield_["exact"]) + # ORD expects YIELD as a percentage (validations.py warns otherwise); + # UDM's YIELD is already percentage-scale. + measurement.percentage.value = value + if precision: + measurement.percentage.precision = precision def _add_provenance( @@ -752,8 +758,11 @@ def _add_provenance( pb2_reaction.provenance.doi = citation["DOI"] break - if "CITATIONS" in reaction: - reaction_citation = _as_list(reaction["CITATIONS"])[0]["CITATION"] + # reaction.get("CITATIONS") may be None (empty ); its + # "CITATION" child, not CITATIONS itself, is the repeatable element. + reaction_citations = _as_list((reaction.get("CITATIONS") or {}).get("CITATION")) + if reaction_citations: + reaction_citation = reaction_citations[0] if "DOI" in reaction_citation: pb2_reaction.provenance.doi = reaction_citation["DOI"] if "PATENT_NUMBER" in reaction_citation: diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index 220497cd..71e84617 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -146,7 +146,7 @@ def test_simple(tmp_path): [product] = outcome.products assert product.identifiers[0].value == "Product A" [measurement] = product.measurements - assert measurement.float_value.value == 85.0 + assert measurement.percentage.value == 85.0 # Off by default: no raw UDM XML embedded without --include-udm-xml. assert "udm_reaction_xml" not in reaction.provenance.reaction_metadata @@ -352,7 +352,7 @@ def test_multiple_condition_groups_are_recorded_as_dynamic(tmp_path): # Only the outcome recorded once in the source, not duplicated. assert len(reaction.outcomes) == 1 - assert reaction.outcomes[0].products[0].measurements[0].float_value.value == 85.0 + assert reaction.outcomes[0].products[0].measurements[0].percentage.value == 85.0 assert reaction.conditions.conditions_are_dynamic # No single-step snapshot: none of the three stages is "the" temperature. @@ -609,6 +609,88 @@ def test_unknown_molecule_reference_does_not_crash(tmp_path): assert component.identifiers[0].value == "does_not_exist" +def test_yield_min_max_range_is_not_dropped(tmp_path): + """Regression test: a YIELD given as / (no ) must still + produce a measurement, using percentage (not float_value) so it's + readable via message_helpers.get_product_yield(), the standard accessor. + """ + reaction_xml = """ + + + + Product A + 8090 + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + [measurement] = dataset.reactions[0].outcomes[0].products[0].measurements + assert measurement.percentage.value == 85.0 + assert measurement.percentage.precision == 5.0 + assert ( + message_helpers.get_product_yield(dataset.reactions[0].outcomes[0].products[0]) + == 85.0 + ) + + +def test_reaction_level_citations_with_multiple_entries(tmp_path): + """Regression test: a reaction-level with more than one + child must still resolve a DOI/patent from the first one, + not silently drop them because the wrong dict level got _as_list'd. + """ + reaction_xml = """ + + + 10.0000/firstUS123 + 10.0000/second + + + + Reactant A + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + assert dataset.reactions[0].provenance.doi == "10.0000/first" + assert dataset.reactions[0].provenance.patent == "US123" + + +def test_reaction_level_empty_citations_does_not_crash(tmp_path): + """Regression test: a self-closing must not crash the + conversion with an IndexError. + """ + reaction_xml = """ + + + + + Reactant A + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + assert len(dataset.reactions) == 1 + assert not dataset.reactions[0].provenance.doi + + def test_missing_input_file(tmp_path): argv = ["--input", str(tmp_path / "does_not_exist.xml")] with pytest.raises(SystemExit): From 63d96fa1cf9874efe8d111353b8cd2b169238a7a Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 18:06:30 +0100 Subject: [PATCH 12/15] Fix PRODUCT_ID spurious/duplicate ReactionOutcome bugs 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 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 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 --- ord_schema/scripts/convert_udm_to_ord.py | 33 +++++--- ord_schema/scripts/convert_udm_to_ord_test.py | 77 +++++++++++++++++++ 2 files changed, 101 insertions(+), 9 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index cb215175..4d193ae8 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -427,8 +427,12 @@ def _build_base_reaction( """Builds the reaction-level data shared by every variation of `reaction`. Covers the UDM fields that live directly on rather than on a - specific : REACTANT_ID/PRODUCT_ID placeholders, RXNSTRUCTURE + specific : the REACTANT_ID placeholder, RXNSTRUCTURE identifiers, and (if requested via --include-udm-xml) the raw source XML. + PRODUCT_ID is handled per-variation instead (see _add_variation), since + it needs to know which product molecules that variation's own + entries already cover, to avoid double-counting one product as two + ReactionOutcomes. """ base_reaction = reaction_pb2.Reaction() @@ -469,14 +473,6 @@ def _build_base_reaction( type=ordtype, details=orddetails, value=structure["value"] ) - if "PRODUCT_ID" in reaction: - outcome = base_reaction.outcomes.add() - for product_id in _as_list(reaction["PRODUCT_ID"]): - identifier = outcome.products.add().identifiers.add( - type="CUSTOM", details="PRODUCT_ID from UDM" - ) - identifier.value = product_id - return base_reaction @@ -539,9 +535,28 @@ def _add_variation( if comment: pb2_reaction.observations.add().comment = comment + # PRODUCT_ID is a reaction-level placeholder (bare molecule ID, no + # structure/yield); PRODUCT is the fuller variation-level data. Track + # which molecules this variation's own PRODUCT entries already cover so + # a PRODUCT_ID for the same molecule isn't recorded as a second, + # duplicate ReactionOutcome. + covered_product_ids = set() for udm_product in _as_list(variation.get("PRODUCT")): + molecule = udm_product.get("MOLECULE") + if molecule and molecule.get("@MOL_ID"): + covered_product_ids.add(molecule["@MOL_ID"]) _add_product(pb2_reaction, all_molecules, udm_product) + for product_id in _as_list(reaction.get("PRODUCT_ID")): + if product_id in covered_product_ids: + continue + identifier = ( + pb2_reaction.outcomes.add() + .products.add() + .identifiers.add(type="CUSTOM", details="PRODUCT_ID from UDM") + ) + identifier.value = product_id + _add_provenance( pb2_reaction, reaction, diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index 71e84617..407ee1a9 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -588,6 +588,83 @@ def test_single_reactant_id_is_not_split_into_characters(tmp_path): assert component.identifiers[0].value == "abc123" +def test_empty_product_id_does_not_add_spurious_outcome(tmp_path): + """Regression test: a self-closing must not add an empty + ReactionOutcome with zero products. + """ + reaction_xml = """ + + + + + Reactant A + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + assert len(dataset.reactions[0].outcomes) == 0 + + +def test_product_id_does_not_duplicate_a_covered_product(tmp_path): + """Regression test: a reaction-level PRODUCT_ID for a molecule already + covered by the variation's own must not add a second, + duplicate ReactionOutcome for the same product. + """ + reaction_xml = """ + + m2 + + + Reactant A + + + Product A + 85.0 + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + [outcome] = dataset.reactions[0].outcomes + [product] = outcome.products + assert product.measurements[0].percentage.value == 85.0 + + +def test_product_id_still_recorded_when_not_covered_by_product(tmp_path): + """A PRODUCT_ID for a molecule the variation's own PRODUCT list doesn't + mention is still recorded as its own placeholder outcome. + """ + reaction_xml = """ + + m2 + + + Reactant A + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + [outcome] = dataset.reactions[0].outcomes + assert outcome.products[0].identifiers[0].value == "m2" + + def test_unknown_molecule_reference_does_not_crash(tmp_path): reaction_xml = """ From 072b4d675339a0e373eb86f1726f1455b8089d02 Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 18:11:00 +0100 Subject: [PATCH 13/15] Fix role-collision input merging and misleading "not found" message 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 " 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 --- ord_schema/scripts/convert_udm_to_ord.py | 24 +++++- ord_schema/scripts/convert_udm_to_ord_test.py | 82 ++++++++++++++++++- 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 4d193ae8..5695ca47 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -579,7 +579,18 @@ def _add_component( if "MOLECULE" not in entry: return mol_id = entry["MOLECULE"]["@MOL_ID"] - component = pb2_reaction.inputs[mol_id].components.add() + # ReactionInput map keys are just descriptions (see reaction.proto), but + # components sharing one ReactionInput are modeled as added together in + # one addition event. If this MOL_ID already has a component under a + # *different* role, reuse would misrepresent two separate additions + # (e.g. the same compound used as both a REAGENT and a CATALYST, with + # distinct amounts) as one; disambiguate the key in that case only, so + # the common (non-colliding) case keeps the plain MOL_ID key. + key = mol_id + existing = pb2_reaction.inputs.get(mol_id) + if existing and any(c.reaction_role != role for c in existing.components): + key = f"{mol_id}_{role_label}" + component = pb2_reaction.inputs[key].components.add() _set_molecule_identifier(component, all_molecules, mol_id, role_label) component.reaction_role = role if "AMOUNT" in entry: @@ -611,6 +622,17 @@ def _set_molecule_identifier( type="CUSTOM", details=f"{role_label} MOLECULE NAME from UDM" ) identifier.value = molval["name"] + elif molval is not None: + # The molecule exists in but has neither a name nor a + # structure -- distinct from mol_id not being in at all. + identifier = component.identifiers.add( + type="CUSTOM", + details=( + f"{role_label} MOL_ID from UDM (found in but has " + "no NAME or MOLSTRUCTURE)" + ), + ) + identifier.value = mol_id else: identifier = component.identifiers.add( type="CUSTOM", diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index 407ee1a9..0c246018 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -19,7 +19,7 @@ import pytest from ord_schema import message_helpers -from ord_schema.proto import dataset_pb2 +from ord_schema.proto import dataset_pb2, reaction_pb2 from ord_schema.scripts import convert_udm_to_ord _MOLECULES = """ @@ -686,6 +686,86 @@ def test_unknown_molecule_reference_does_not_crash(tmp_path): assert component.identifiers[0].value == "does_not_exist" +def test_molecule_found_but_empty_is_not_mislabeled_as_missing(tmp_path): + """Regression test: a present in but with neither + a NAME nor a MOLSTRUCTURE must not be labeled "not found in " + -- it was found, it's just empty, which is a different, distinguishable + problem for anyone debugging conversion gaps. + """ + molecules = """ + + + + """ + reaction_xml = """ + + + + Reactant A + + + +""" + udm_xml = f""" + Test Dataset + {molecules} + {reaction_xml} + +""" + input_filename = _write(tmp_path, "input.xml", udm_xml) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + [component] = dataset.reactions[0].inputs["m1"].components + details = component.identifiers[0].details + assert "not found in " not in details + assert "found in but has no NAME or MOLSTRUCTURE" in details + + +def test_same_molecule_in_two_roles_does_not_merge_inputs(tmp_path): + """Regression test: the same MOL_ID used under two different roles in one + variation (e.g. as both a REAGENT and a CATALYST) must not collapse into + a single ReactionInput, which would misrepresent two separate additions + (with distinct amounts) as one addition event. + """ + reaction_xml = """ + + + + Reactant A + 1.0 + + + Reactant A + 0.05 + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + # The first-seen role (REAGENT) keeps the plain MOL_ID key; the second, + # different role (CATALYST) gets a disambiguated key instead of merging + # into the same ReactionInput. + assert len(reaction.inputs) == 2 + [reagent_component] = reaction.inputs["m1"].components + assert reagent_component.reaction_role == reaction_pb2.ReactionRole.REAGENT + assert reagent_component.amount.moles.value == 1.0 + + [other_key] = [key for key in reaction.inputs if key != "m1"] + assert other_key == "m1_CATALYST" + [catalyst_component] = reaction.inputs[other_key].components + assert catalyst_component.reaction_role == reaction_pb2.ReactionRole.CATALYST + assert catalyst_component.amount.moles.value == pytest.approx(0.05) + + def test_yield_min_max_range_is_not_dropped(tmp_path): """Regression test: a YIELD given as / (no ) must still produce a measurement, using percentage (not float_value) so it's From d19af5463da062b696f0b2dd5cfdd5ff59fd551b Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 19:41:49 +0100 Subject: [PATCH 14/15] Stop assuming a lone min/max range bound is an exact value Greptile flagged this in two places: _add_conditions (single condition group) wrote a lone 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 100. "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 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 written to setpoint.value as if exact -- and switched its fixture to a complete min/max pair (the only form the XSD allows alongside 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 --- ord_schema/scripts/convert_udm_to_ord.py | 63 ++++++++--- ord_schema/scripts/convert_udm_to_ord_test.py | 102 +++++++++++++++++- 2 files changed, 147 insertions(+), 18 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 5695ca47..10101622 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -265,14 +265,23 @@ def _text_and_unit(value: Any) -> tuple[str | None, str | None]: def _parse_range(value: UdmDict) -> tuple[float, float | None] | None: - """Parses a UDM min/max/exact range into (midpoint, precision). + """Parses a UDM min/max/exact range into a point value: (midpoint, precision). UDM's *Range types (temperatureRange, pressureRange, etc.) hold exactly one of {min, max, min and max, exact} -- see Docs/ChangeLog.md's note on XML Schema's limited support for "exactly one of" constraints. `exact` maps to (value, None); a min/max pair maps to their midpoint with - precision = half the spread; a lone min or max maps to that value with - no precision. Returns None if none of min/max/exact is present. + precision = half the spread. + + A *lone* min or max is deliberately NOT treated as a point value here: + "at most 100 degC" is not the same claim as "exactly 100 degC", and + ORD's Temperature/Pressure/etc. messages have no field for a one-sided + bound, only value+precision. Returning it as (100, None) -- identical + to what an actual 100 produces -- would silently turn a + bound into a fabricated exact reading. Callers that only need a + complete/exact point value should use this function directly and treat + None as "not capturable structurally"; _format_range_text (below) + additionally renders a one-sided bound as labeled text. """ if "exact" in value: return float(value["exact"]), None @@ -280,23 +289,31 @@ def _parse_range(value: UdmDict) -> tuple[float, float | None] | None: if min_v is not None and max_v is not None: min_f, max_f = float(min_v), float(max_v) return (min_f + max_f) / 2, (max_f - min_f) / 2 - if min_v is not None: - return float(min_v), None - if max_v is not None: - return float(max_v), None return None def _format_range_text(value: UdmDict, default_unit: str) -> str | None: - """Renders a UDM min/max/exact range as human-readable text, e.g. "20±5 degC".""" - parsed = _parse_range(value) - if parsed is None: - return None - midpoint, precision = parsed + """Renders a UDM min/max/exact range as human-readable text. + + A complete range or exact value renders as "20±5 degC" / "20 degC". A + lone min or max -- which _parse_range deliberately does not turn into a + point value, see its docstring -- renders as a labeled bound instead, + e.g. ">=20 degC" or "<=100 degC", so it's never confused with an exact + reading. + """ unit = value.get("@unit", default_unit) - if precision: - return f"{midpoint}±{precision} {unit}".strip() - return f"{midpoint} {unit}".strip() + parsed = _parse_range(value) + if parsed is not None: + midpoint, precision = parsed + if precision: + return f"{midpoint}±{precision} {unit}".strip() + return f"{midpoint} {unit}".strip() + min_v, max_v = value.get("min"), value.get("max") + if min_v is not None: + return f">={min_v} {unit}".strip() + if max_v is not None: + return f"<={max_v} {unit}".strip() + return None def _format_incr(value: UdmDict) -> str | None: @@ -723,7 +740,8 @@ def _add_product( _set_molecule_identifier( component, all_molecules, molecule.get("@MOL_ID"), "PRODUCT" ) - parsed = _parse_range(udm_product.get("YIELD", {})) + yield_ = udm_product.get("YIELD", {}) + parsed = _parse_range(yield_) if parsed: value, precision = parsed measurement = component.measurements.add() @@ -733,6 +751,19 @@ def _add_product( measurement.percentage.value = value if precision: measurement.percentage.precision = precision + else: + # A lone or ("at least"/"at most" X%) isn't a point + # value _parse_range will produce (see its docstring) -- ORD's + # Percentage has no field for a one-sided bound, so it's recorded + # as text via .details instead of being fabricated as exact or + # silently dropped. + text = _format_range_text(yield_, "percent") + if text: + measurement = component.measurements.add() + measurement.type = ( + reaction_pb2.ProductMeasurement.ProductMeasurementType.YIELD + ) + measurement.details = f"yield {text}" def _add_provenance( diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index 0c246018..f619f5d2 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -478,6 +478,9 @@ def test_temperature_ramp_incr_is_not_dropped(tmp_path): single-group case where the central value *is* captured structurally (Temperature.setpoint) -- excluding all of TEMPERATURE from the "extra" text pass would silently drop incr along with it. + + Uses a complete / pair rather than , since the XSD's + temperatureRange only allows alongside min/max, never . """ reaction_xml = """ @@ -488,7 +491,8 @@ def test_temperature_ramp_incr_is_not_dropped(tmp_path): - 100 + 95 + 105 5 @@ -503,12 +507,79 @@ def test_temperature_ramp_incr_is_not_dropped(tmp_path): dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) reaction = dataset.reactions[0] - # Central value still structural. + # Central value still structural: a complete min/max pair is a point + # value (midpoint + precision), unlike a lone bound (see the next test). assert reaction.conditions.temperature.setpoint.value == 100.0 + assert reaction.conditions.temperature.setpoint.precision == 5.0 # Ramp rate, which has no structured home, surfaces as text instead. assert "temperature ramp=5 deg_C/hour" in reaction.conditions.details +def test_lone_bound_is_not_treated_as_an_exact_value(tmp_path): + """Regression test: a lone (no , no ) means "at most + X", not "exactly X" -- it must not be written to Temperature.setpoint + as if it were an exact reading, and must still be recoverable as + labeled text rather than silently dropped. + """ + reaction_xml = """ + + + + Reactant A + + + + 100 + + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + # Not fabricated as an exact 100 degC setpoint. + assert reaction.conditions.temperature.setpoint.value == 0.0 + # But not silently dropped either -- recorded as a labeled bound. + assert "temperature <=100 degC" in reaction.conditions.details + + +def test_lone_bound_in_dynamic_stage_is_labeled(tmp_path): + """Same lone-bound distinction, but in the multi-group (dynamic) text + summary, which is the only record of each stage. + """ + reaction_xml = """ + + + + Reactant A + + + + 2020 + + + 90 + + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + details = dataset.reactions[0].conditions.details + assert "Stage 1: temperature 20.0 degC" in details + assert "Stage 2: temperature >=90 degC" in details + + def test_non_rpm_stirring_is_not_dropped(tmp_path): """Regression test: STIRRING in a unit other than rpm has no structured home (StirringConditions.rate.rpm is rpm-specific), so it must surface @@ -796,6 +867,33 @@ def test_yield_min_max_range_is_not_dropped(tmp_path): ) +def test_yield_lone_bound_is_not_fabricated_as_exact(tmp_path): + """Regression test: a YIELD given as a lone ("at least X%") must + not be written to Percentage.value as if it were an exact reading -- + Percentage has no field for a one-sided bound, so it's recorded as text + via ProductMeasurement.details instead. + """ + reaction_xml = """ + + + + Product A + 80 + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + [measurement] = dataset.reactions[0].outcomes[0].products[0].measurements + assert not measurement.HasField("percentage") + assert measurement.details == "yield >=80 percent" + + def test_reaction_level_citations_with_multiple_entries(tmp_path): """Regression test: a reaction-level with more than one child must still resolve a DOI/patent from the first one, From 8e01efc06c5040645e41eb1f9f09508680e2c1e6 Mon Sep 17 00:00:00 2001 From: Ben Deadman Date: Fri, 14 Aug 2026 22:13:51 +0100 Subject: [PATCH 15/15] Stop assuming STIRRING is always captured structurally, take two Greptile flagged that an rpm STIRRING given as a / 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 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 --- ord_schema/scripts/convert_udm_to_ord.py | 11 +++- ord_schema/scripts/convert_udm_to_ord_test.py | 66 +++++++++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/ord_schema/scripts/convert_udm_to_ord.py b/ord_schema/scripts/convert_udm_to_ord.py index 10101622..feede92b 100644 --- a/ord_schema/scripts/convert_udm_to_ord.py +++ b/ord_schema/scripts/convert_udm_to_ord.py @@ -703,11 +703,18 @@ def _add_conditions( stirring = condition_group.get("STIRRING", {}) parsed = _parse_range(stirring) if parsed and stirring.get("@unit", "rpm") == "rpm": - rpm = round(parsed[0]) + value, precision = parsed + rpm = round(value) pb2_reaction.conditions.stirring.type = reaction_pb2.StirringConditions.CUSTOM pb2_reaction.conditions.stirring.details = f"{rpm} rpm" pb2_reaction.conditions.stirring.rate.rpm = rpm - captured.add("STIRRING") + # rate.rpm is a bare int with no precision field, unlike + # Temperature/Pressure's setpoint -- a real min/max spread (e.g. + # 490-510) would be silently rounded away to 500 with no trace + # unless STIRRING is left out of `captured` here, letting it fall + # through to the text fallback, which does render the full range. + if not precision: + captured.add("STIRRING") if "REFLUX" in condition_group: pb2_reaction.conditions.reflux = _parse_bool(condition_group["REFLUX"]) diff --git a/ord_schema/scripts/convert_udm_to_ord_test.py b/ord_schema/scripts/convert_udm_to_ord_test.py index f619f5d2..103d1823 100644 --- a/ord_schema/scripts/convert_udm_to_ord_test.py +++ b/ord_schema/scripts/convert_udm_to_ord_test.py @@ -616,6 +616,72 @@ def test_non_rpm_stirring_is_not_dropped(tmp_path): assert "stirring 50.0 Hz" in reaction.conditions.details +def test_stirring_range_precision_is_not_dropped(tmp_path): + """Regression test: an rpm STIRRING given as a / range must not + be silently rounded down to a bare exact value -- StirringConditions. + rate.rpm has no precision field (unlike Temperature/Pressure's + setpoint), so the range would otherwise vanish, leaving 490-510 rpm + indistinguishable from an exact 500 rpm. + """ + reaction_xml = """ + + + + Reactant A + + + + 490510 + + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + # Best-effort structural value: the rounded midpoint. + assert reaction.conditions.stirring.rate.rpm == 500 + # But the range itself isn't lost -- it's still in the text fallback, + # as midpoint and precision (avoid asserting the literal "±" glyph). + assert "stirring 500.0" in reaction.conditions.details + assert "10.0 rpm" in reaction.conditions.details + + +def test_stirring_exact_value_is_not_duplicated_as_text(tmp_path): + """An exact (or degenerate min==max) rpm STIRRING loses nothing by being + rounded, so it should be captured structurally only, not also repeated + in the text fallback. + """ + reaction_xml = """ + + + + Reactant A + + + + 500 + + + + +""" + input_filename = _write(tmp_path, "input.xml", _udm_xml(reaction_xml)) + output_filename = str(tmp_path / "output.pbtxt") + argv = ["--input", input_filename, "--output", output_filename, "--no-validate"] + convert_udm_to_ord.main(convert_udm_to_ord.parse_args(argv)) + + dataset = message_helpers.load_message(output_filename, dataset_pb2.Dataset) + reaction = dataset.reactions[0] + assert reaction.conditions.stirring.rate.rpm == 500 + assert "stirring" not in reaction.conditions.details + + def test_reaction_without_variation_still_emits_one_reaction(tmp_path): reaction_xml = """