microsoft / syntheseus

Package for Retrosynthetic Planning
https://microsoft.github.io/syntheseus/
MIT License
114 stars 15 forks source link

Use unified reaction class in both `search` and `reaction_prediction` #67

Closed AustinT closed 8 months ago

AustinT commented 9 months ago

This PR integrates the new reaction classes from #63 throughout the code. This involved:

  1. Replacing imports in the search code. This was relatively minimal since the new class had the same attributes as the previous search-specific class.
  2. Replacing imports and reaction creation in the reaction_prediction code. This was slightly more challenging and required the following changes:
    1. Redefine type generics for reaction model. Previously it was [InputType, OutputType]. However, reactions no longer have input and output fields as of #63. Therefore I changed this to [InputType, ReactionType]. Note that typing aside, this does not change the behaviour of the models.
    2. Refactored single-step models to return reactions of the same type. Most of the models are backward only so this change was easy, but Chemformer works in both directions so it was slightly more challenging. I had to introduce a separate post-processing method which returns MultiProductReactions instead of SingleProductReactions.
    3. Eval script: had to do a decent amount of refactoring here since the core code essentially checked prediction.output == ground_truth, which is no longer possible if reactions are not labelled with explicit input and output fields. I largely solved this by just checking for equality of the reaction objects directly. Typing of the back translation model was also a bit awkward. I resolved this by hard-coding the back translation model to be a forward model instead of leaving it generic (i.e. ReactionModel[Bag[Molecule], MultiProductReaction]). I think this is fine because only forward "back translation" models are supported anyway (there is an explicit if statement checking for this).
  3. Deleted the search.chem.BackwardReaction class and interface.models.Prediction classes since they are no longer used.
  4. Moved old tests for these methods into test_molecule.py and test_reaction.py. This also required making a minimal top-level conftest.py file for some pytest fixtures referenced in these tests.
AustinT commented 9 months ago

@kmaziarz this PR is mostly minor refactoring changes. The only part which probably needs a more detailed review is eval_single_step.py.