From 6432a714be3126a6134f36041e98e6fc48fd0b2e Mon Sep 17 00:00:00 2001 From: Austin Tripp Date: Fri, 8 Sep 2023 11:02:13 +0100 Subject: [PATCH] Fix error in graph standardization (#24) The initial release included a file `syntheseus/search/graph/standardization.py` which contained code to turn all types of graphs into minimal AND/OR graphs (i.e. AND/OR graphs with one node per reaction and one node per molecule). It turns out that for MolSetGraph objects the code to fetch the reaction objects was incorrect and would crash. I didn't catch this before because I didn't actually write tests for this file. This PR fixes this bug, and adds minimal "smoke" tests to make sure that this method runs without crashing. However, the actual output is not tested. To reflect this, the method still raises a warning when it is called. --------- Co-authored-by: Krzysztof Maziarz --- CHANGELOG.md | 1 + syntheseus/search/graph/standardization.py | 2 +- .../search/graph/test_standardization.py | 24 ++++++++++++++++++- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53c5af40..012eecf7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0. ### Fixed +- Fix bug where standardizing MolSetGraphs crashed ([#24](https://github.com/microsoft/syntheseus/pull/24)) ([@austint]) - Change default node depth to infinity ([#16](https://github.com/microsoft/syntheseus/pull/16)) ([@austint]) - Adapt tutorials to the renaming from PR #9 ([#17](https://github.com/microsoft/syntheseus/pull/17)) ([@jagarridotorres]) - Pin `pydantic` version to `1.*` ([#10](https://github.com/microsoft/syntheseus/pull/10)) ([@kmaziarz]) diff --git a/syntheseus/search/graph/standardization.py b/syntheseus/search/graph/standardization.py index 1c98b3c4..507ff457 100644 --- a/syntheseus/search/graph/standardization.py +++ b/syntheseus/search/graph/standardization.py @@ -106,7 +106,7 @@ def _unique_node_andor_from_molset( # Get all mols and reactions all_nodes: set[MolSetNode] = set(graph._graph.nodes()) mols = set(mol for node in all_nodes for mol in node.mols) - rxns = set(edge["reaction"] for edge in graph._graph.edges()) + rxns = set(graph._graph.edges[n1, n2]["reaction"] for n1, n2 in graph._graph.edges()) # Make initial nodes mol_to_node = {mol: OrNode(mol) for mol in mols} diff --git a/syntheseus/tests/search/graph/test_standardization.py b/syntheseus/tests/search/graph/test_standardization.py index 96ef8932..251256db 100644 --- a/syntheseus/tests/search/graph/test_standardization.py +++ b/syntheseus/tests/search/graph/test_standardization.py @@ -1 +1,23 @@ -"""Graph standardization is not tested at this time, but should be tested in the future.""" +""" +At the moment, only a smoke-test is done for graph standardization, +but the exact behaviour is not well-tested. +These tests should be added in the future. +""" + +import pytest + +from syntheseus.search.graph.and_or import AndOrGraph +from syntheseus.search.graph.molset import MolSetGraph +from syntheseus.search.graph.standardization import get_unique_node_andor_graph + + +def test_smoke_andor(andor_graph_non_minimal: AndOrGraph): + with pytest.warns(UserWarning): + output = get_unique_node_andor_graph(andor_graph_non_minimal) + + assert len(output) == len(andor_graph_non_minimal) # no nodes deleted here + + +def test_smoke_molset(molset_tree_non_minimal: MolSetGraph): + with pytest.warns(UserWarning): + get_unique_node_andor_graph(molset_tree_non_minimal)