ReactionMechanismGenerator / ARC

ARC - Automatic Rate Calculator
https://reactionmechanismgenerator.github.io/ARC/index.html
MIT License
42 stars 21 forks source link

Fixed: allow mutiple scissions in the atom mapping process. #701

Closed kfir4444 closed 9 months ago

kfir4444 commented 9 months ago

This PR presents a new method of performing the scission in atom mapping, based on the atom indices, which are required for atom mapping anyway. This method also simplify the process. Tests were also added.

codecov[bot] commented 9 months ago

Codecov Report

Merging #701 (5bfeaac) into main (5c3aab6) will decrease coverage by 0.01%. The diff coverage is 93.70%.

@@            Coverage Diff             @@
##             main     #701      +/-   ##
==========================================
- Coverage   73.30%   73.29%   -0.01%     
==========================================
  Files          99       99              
  Lines       26609    26557      -52     
  Branches     5563     5537      -26     
==========================================
- Hits        19506    19466      -40     
+ Misses       5759     5744      -15     
- Partials     1344     1347       +3     
Flag Coverage Δ
unittests 73.29% <93.70%> (-0.01%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
arc/mapping/engine_test.py 99.56% <100.00%> (-0.01%) :arrow_down:
arc/mapping/driver.py 70.27% <71.42%> (-1.29%) :arrow_down:
arc/reaction_test.py 99.33% <86.66%> (-0.26%) :arrow_down:
arc/mapping/engine.py 88.25% <90.00%> (+0.26%) :arrow_up:

... and 3 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

alongd commented 9 months ago

I tried to map the following rxn on this branch C=CC([O])[CH]C=CCC <=> CCC=C[CH]CC=C[O]

and got an error:

IndexError                                Traceback (most recent call last)
/tmp/ipykernel_225830/1280115450.py in <module>
----> 1 print(rxn1.atom_map)

~/Code/ARC/arc/reaction.py in atom_map(self)
    156                 and all(species.get_xyz(generate=False) is not None for species in self.r_species + self.p_species):
    157             for backend in ["ARC", "QCElemental"]:
--> 158                 _atom_map = map_reaction(rxn=self, backend=backend)
    159                 if _atom_map is not None:
    160                     self._atom_map = _atom_map

~/Code/ARC/arc/mapping/driver.py in map_reaction(rxn, backend, db, flip)
     71             return _map if _map is not None else map_reaction(rxn, backend=backend, db=db, flip=True)
     72         try:
---> 73             _map = map_rxn(rxn, backend=backend, db=db)
     74         except ValueError as e:
     75             return map_reaction(rxn, backend=backend, db=db, flip=True)

~/Code/ARC/arc/mapping/driver.py in map_rxn(rxn, backend, db)
    237     rmg_reactions = get_rmg_reactions_from_arc_reaction(arc_reaction=rxn, backend=backend)
    238     r_label_dict, p_label_dict = get_atom_indices_of_labeled_atoms_in_an_rmg_reaction(arc_reaction=rxn,
--> 239                                                                                       rmg_reaction=rmg_reactions[0])
    240 
    241     # step 2:

IndexError: list index out of range

Can you take a look?

alongd commented 9 months ago

BTW, I think this is the correct mapping: image

kfir4444 commented 9 months ago

C=CC([O])[CH]C=CCC <=> CCC=C[CH]CC=C[O]

Did it previously work on the main branch? I think I know the issue, and I did not change the function that returns a wrong value. Also, I this this issue probably stem from the fact that get_rmg_reactions_from_arc_reaction(arc_reaction=rxn, backend=backend) for this reaction returns an empty list. Also, could you share the full script, and maybe also check that the reaction family is found correctly?