search

microsoft / syntheseus

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

Add new compromise reaction class #63

Closed AustinT closed 8 months ago

AustinT commented 8 months ago

This PR adds a new compromise Reaction class in a new file: syntheseus/interface/reaction.py. The class has the features we discussed in #58 [^1]. The class also includes a basic test.

The new classes is not used anywhere at the moment[^2]. I will refactor the rest of the code to use it in another PR. I thought it would be test to propose the class in isolation first to avoid doing all the refactoring, then re-doing it if we decide to make additional changes.

[^1]: Except for the unique_reactants alias. After thinking about it a bit more, if users know to use unique_reactants then they could just as easily write set(reactants).

[^2]: Except that I moved the REACTION_SEPARATOR constant to reaction.py and changed resulting imports. It seemed odd to have it in molecule.py

AustinT commented 8 months ago

If users know to use unique_reactants then they could just as easily write set(reactants).

This may be true, but I think another advantage of having unique_reactants is that it makes it more clear the reactants are not unique. Otherwise I could imagine someone knowing they need unique reactants, but not realizing reactants are not already unique... Apart from that LGTM.

Good point. I added them to the base class. Do you think that is the appropriate place to add them @kmaziarz? There is some redundancy between the implementation in MultiProductReaction and SingleProductReaction but it is very small.

I considered adding it just to SingleProductReaction but that could create typing issues because general Reaction objects will not necessarily have this method. I also considered making a shared base class between MultiProductReaction and SingleProductReaction (e.g. _BagReactantsReaction) but I thought that would reduce readability.

kmaziarz commented 8 months ago

I added them to the base class. Do you think that is the appropriate place to add them @kmaziarz? There is some redundancy between the implementation in MultiProductReaction and SingleProductReaction but it is very small.

Yes, what you've done seems reasonable.

AustinT commented 8 months ago

I added them to the base class. Do you think that is the appropriate place to add them @kmaziarz? There is some redundancy between the implementation in MultiProductReaction and SingleProductReaction but it is very small.

Yes, what you've done seems reasonable.

Sounds good, I'll merge this then.