Auto-Mech / mechanalyzer

Tools construction, manipulation, visualization of chemical mechanisms
MIT License
3 stars 15 forks source link

rxn class graphs "unclassified" vs previous classification as H migration or substitution #294

Closed lpratalimaffei closed 2 months ago

lpratalimaffei commented 2 months ago

@avcopan @snelliott when re-testing mechanalyzer I found three cases of sorting of reactions by class (from reaction line only, so let's say from the InChIs) that stopped working. In the test, I commented the line that I got previously. these are the three cases

https://github.com/Auto-Mech/mechanalyzer/blob/777991b8330d0e85bd544b9cec55b75949f6d192/mechanalyzer/tests/test__sorter.py#L301

https://github.com/Auto-Mech/mechanalyzer/blob/777991b8330d0e85bd544b9cec55b75949f6d192/mechanalyzer/tests/test__sorter.py#L304

https://github.com/Auto-Mech/mechanalyzer/blob/777991b8330d0e85bd544b9cec55b75949f6d192/mechanalyzer/tests/test__sorter.py#L307

here is where I call the reaction classifier within the sorter: https://github.com/Auto-Mech/mechanalyzer/blob/777991b8330d0e85bd544b9cec55b75949f6d192/mechanalyzer/builder/rxnclass.py#L261

could you give a look at this to make sure this is not an error? I would expect the H migration to work at least. the substitution is probably more debatable. thanks!

avcopan commented 2 months ago

@lpratalimaffei I am testing these and the first one seems to be correctly recognized by AutoMol:

>>> import automol
>>> rxn, *_ = automol.reac.from_smiles(["[CH2]C=C"], ["[CH]=CC"], stereo=False)
>>> automol.reac.display(rxn)

Screenshot 2024-08-20 095413

>>> print(automol.reac.class_(rxn))
hydrogen migration
avcopan commented 2 months ago

I will try testing it directly as in the link you sent...

avcopan commented 2 months ago

The hydrogen migration seems to work when called directly through mechanalyzer as well:

>>> import mechanalyzer.builder.rxnclass
>>> 
>>> spc_dct = {'C3H5-A': {'inchi': 'InChI=1S/C3H5/c1-3-2/h3H,1-2H2', 'fml': {'C': 3, 'H': 5}},
>>>            'C3H5-S': {'inchi': 'InChI=1S/C3H5/c1-3-2/h1,3H,2H3', 'fml': {'C': 3, 'H': 5}}}
>>> mechanalyzer.builder.rxnclass.classify_graph(spc_dct, ('C3H5-A',), ('C3H5-S',))
'hydrogen migration'
avcopan commented 2 months ago

The "substitutions" are no longer classified as such. We have made this class more strict, because it was giving too many false positives.

lpratalimaffei commented 2 months ago

thank you!! so I checked the InChI for C3H5-S in the data; we had stereo-expanded the spc dct so it is AMChI=1/C3H5/c1-3-2/h1,3H,2H3/b3-1- so I guess this is incompatible with C3H5-A? should I set stereo=False in generating the rxn object?

avcopan commented 2 months ago

Hey @lpratalimaffei -- yes, in general, it is best to classify first and then add stereo after. If you classify with stereo=True, then the classifier will include stereo parities in the comparison.

avcopan commented 2 months ago

I think this is resolved, so I will mark it as closed.

lpratalimaffei commented 2 months ago

Great, thank you. Probably when we introduced stereo I didn’t pay attention to this and kept the default option. I’ll switch to stereo=False in the next commit.

Da: avcopan @.> Inviato: mercoledì 21 agosto 2024 15:18 A: Auto-Mech/mechanalyzer @.> Cc: Luna Pratali Maffei @.>; Mention @.> Oggetto: Re: [Auto-Mech/mechanalyzer] rxn class graphs "unclassified" vs previous classification as H migration or substitution (Issue #294)

Hey @lpratalimaffeihttps://github.com/lpratalimaffei -- yes, in general, it is best to classify first and then add stereo after. If you classify with stereo=True, then the classifier will include stereo parities in the comparison.

— Reply to this email directly, view it on GitHubhttps://github.com/Auto-Mech/mechanalyzer/issues/294#issuecomment-2302038856, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AOPNZNJXRTEVMXD3EOZJAMTZSSHQTAVCNFSM6AAAAABMZMYHLGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBSGAZTQOBVGY. You are receiving this because you were mentioned.Message ID: @.**@.>>