diff --git a/Code/GraphMol/ChemReactions/ReactionWriter.cpp b/Code/GraphMol/ChemReactions/ReactionWriter.cpp index 8a75a9d73..c881fed41 100644 --- a/Code/GraphMol/ChemReactions/ReactionWriter.cpp +++ b/Code/GraphMol/ChemReactions/ReactionWriter.cpp @@ -119,18 +119,17 @@ std::string ChemicalReactionToRxnSmiles(const ChemicalReaction &rxn, //! returns an RXN block for a reaction std::string ChemicalReactionToV3KRxnBlock(const ChemicalReaction &rxn, bool separateAgents) { - if (separateAgents) { - // V3000 doesn't seem to support agent blocks - BOOST_LOG(rdWarningLog) - << "V3000 RXN blocks do not support separating agents. Agents will be " - "written to the reactants block" - << std::endl; - } std::ostringstream res; res << "$RXN V3000\n\n RDKit\n\n"; - res << "M V30 COUNTS " - << rxn.getNumReactantTemplates() + rxn.getNumAgentTemplates() << " " - << rxn.getNumProductTemplates(); + + if (separateAgents) { + res << "M V30 COUNTS " << rxn.getNumReactantTemplates() << " " + << rxn.getNumProductTemplates() << " " << rxn.getNumAgentTemplates(); + } else { + res << "M V30 COUNTS " + << rxn.getNumReactantTemplates() + rxn.getNumAgentTemplates() << " " + << rxn.getNumProductTemplates(); + } res << "\n"; res << "M V30 BEGIN REACTANT\n"; @@ -140,11 +139,13 @@ std::string ChemicalReactionToV3KRxnBlock(const ChemicalReaction &rxn, MolOps::findSSSR(*rt); res << FileParserUtils::getV3000CTAB(*rt, -1); } - for (const auto &rt : boost::make_iterator_range(rxn.beginAgentTemplates(), - rxn.endAgentTemplates())) { - // to write the mol block, we need ring information: - MolOps::findSSSR(*rt); - res << FileParserUtils::getV3000CTAB(*rt, -1); + if (!separateAgents) { + for (const auto &rt : boost::make_iterator_range(rxn.beginAgentTemplates(), + rxn.endAgentTemplates())) { + // to write the mol block, we need ring information: + MolOps::findSSSR(*rt); + res << FileParserUtils::getV3000CTAB(*rt, -1); + } } res << "M V30 END REACTANT\n"; res << "M V30 BEGIN PRODUCT\n"; @@ -155,6 +156,17 @@ std::string ChemicalReactionToV3KRxnBlock(const ChemicalReaction &rxn, res << FileParserUtils::getV3000CTAB(*rt, -1); } res << "M V30 END PRODUCT\n"; + if (separateAgents) { + res << "M V30 BEGIN AGENT\n"; + for (const auto &rt : boost::make_iterator_range(rxn.beginAgentTemplates(), + rxn.endAgentTemplates())) { + // to write the mol block, we need ring information: + MolOps::findSSSR(*rt); + res << FileParserUtils::getV3000CTAB(*rt, -1); + } + res << "M V30 END AGENT\n"; + } + res << "M END\n"; return res.str(); } diff --git a/Code/GraphMol/ChemReactions/catch_tests.cpp b/Code/GraphMol/ChemReactions/catch_tests.cpp index 26ed329ba..3668a2c58 100644 --- a/Code/GraphMol/ChemReactions/catch_tests.cpp +++ b/Code/GraphMol/ChemReactions/catch_tests.cpp @@ -1269,3 +1269,107 @@ TEST_CASE("CDXML Parser") { CHECK(smarts == "[#6&D2:2]1:[#6&D2:3]:[#6&D2:4]:[#6&D3:1](:[#6&D2:5]:[#6&D2:6]:1)-[#17&D1].[#6&D3](-[#5&D2]-[#6&D3:7]1:[#6&D2:8]:[#6&D2:9]:[#6&D2:10]:[#6&D2:11]:[#6&D2:12]:1)(-[#8&D1])-[#8&D1]>>[#6:1]1=[#6:5]-[#6:6](=[#6:2]-[#6:3]=[#6:4]-1)-[#6:7]1-[#6:8]=[#6:9]-[#6:10]=[#6:11]-[#6:12]=1"); } } + +TEST_CASE("Github #5785: separateAgents ignored for V3000 RXN files") { + SECTION("general separateAgents parse testing: V2000"){ + std::string rxnb = R"RXN($RXN + + RDKit + + 1 1 1 +$MOL + + RDKit 2D + + 2 1 0 0 0 0 0 0 0 0999 V2000 + 0.0000 0.0000 0.0000 C 0 0 0 0 0 0 0 0 0 1 0 0 + 1.2990 0.7500 0.0000 O 0 0 0 0 0 0 0 0 0 2 0 0 + 1 2 1 0 +M END +$MOL + + RDKit 2D + + 2 1 0 0 0 0 0 0 0 0999 V2000 + 0.0000 0.0000 0.0000 C 0 0 0 0 0 0 0 0 0 1 0 0 + 1.2990 0.7500 0.0000 O 0 0 0 0 0 0 0 0 0 2 0 0 + 1 2 2 0 +M END +$MOL + + RDKit 2D + + 1 0 0 0 0 0 0 0 0 0999 V2000 + 0.0000 0.0000 0.0000 Pt 0 0 0 0 0 0 0 0 0 0 0 0 +M END +)RXN"; + std::unique_ptr rxn(RxnBlockToChemicalReaction(rxnb)); + REQUIRE(rxn); + CHECK(rxn->getNumReactantTemplates()==1); + CHECK(rxn->getNumProductTemplates()==1); + CHECK(rxn->getNumAgentTemplates()==1); + + auto orxn = ChemicalReactionToRxnBlock(*rxn,true); + CHECK(orxn.find(" 1 1 1") != std::string::npos); + + } + SECTION("general separateAgents parse testing: V3000"){ + std::string rxnb = R"RXN($RXN V3000 + + Mrv2211 121520220816 + +M V30 COUNTS 1 1 1 +M V30 BEGIN REACTANT +M V30 BEGIN CTAB +M V30 COUNTS 2 1 0 0 0 +M V30 BEGIN ATOM +M V30 1 C -4.8977 -0.385 0 1 +M V30 2 O -3.564 0.385 0 2 +M V30 END ATOM +M V30 BEGIN BOND +M V30 1 1 1 2 +M V30 END BOND +M V30 END CTAB +M V30 END REACTANT +M V30 BEGIN PRODUCT +M V30 BEGIN CTAB +M V30 COUNTS 2 1 0 0 0 +M V30 BEGIN ATOM +M V30 1 C 4.444 -0.385 0 1 +M V30 2 O 5.7777 0.385 0 2 +M V30 END ATOM +M V30 BEGIN BOND +M V30 1 2 1 2 +M V30 END BOND +M V30 END CTAB +M V30 END PRODUCT +M V30 BEGIN AGENT +M V30 BEGIN CTAB +M V30 COUNTS 1 0 0 0 0 +M V30 BEGIN ATOM +M V30 1 Pt 0 1.54 0 0 +M V30 END ATOM +M V30 END CTAB +M V30 END AGENT +M END +)RXN"; +std::unique_ptr rxn(RxnBlockToChemicalReaction(rxnb)); + REQUIRE(rxn); + CHECK(rxn->getNumReactantTemplates()==1); + CHECK(rxn->getNumProductTemplates()==1); + CHECK(rxn->getNumAgentTemplates()==1); + + { // with separate agents + auto orxn = ChemicalReactionToV3KRxnBlock(*rxn,true); + CHECK(orxn.find("COUNTS 1 1 1") != std::string::npos); + CHECK(orxn.find("BEGIN AGENT") != std::string::npos); + CHECK(orxn.find("END AGENT") != std::string::npos); + } + { // without separate agents + auto orxn = ChemicalReactionToV3KRxnBlock(*rxn,false); + CHECK(orxn.find("COUNTS 2 1") != std::string::npos); + CHECK(orxn.find("BEGIN AGENT") == std::string::npos); + CHECK(orxn.find("END AGENT") == std::string::npos); + } + } +} \ No newline at end of file diff --git a/Code/GraphMol/FileParsers/MolFileWriter.cpp b/Code/GraphMol/FileParsers/MolFileWriter.cpp index 75580f2c8..8d8507630 100644 --- a/Code/GraphMol/FileParsers/MolFileWriter.cpp +++ b/Code/GraphMol/FileParsers/MolFileWriter.cpp @@ -530,9 +530,12 @@ bool hasNonDefaultValence(const Atom *atom) { if (atom->getNumRadicalElectrons() != 0) { return true; } - if (atom->hasQuery()) { + // for queries and atoms which don't have computed properties, the answer is + // always no: + if (atom->hasQuery() || atom->needsUpdatePropertyCache()) { return false; } + if (atom->getAtomicNum() == 1 || SmilesWrite ::inOrganicSubset(atom->getAtomicNum())) { // for the ones we "know", we may have to specify the valence if it's