MolecularAI / aizynthfinder

A tool for retrosynthetic planning
https://molecularai.github.io/aizynthfinder/
MIT License
571 stars 130 forks source link

Going over the max transforms limit #89

Closed tagirshin closed 9 months ago

tagirshin commented 1 year ago

Hello! During analysis of tree search results I found out that if I use standard config without specifying max_transforms parameter, the tool can give me routes with 7 transformations instead of standard 6. Here is an example of a found pathway: route000

I tried to read the config file and on this side everything is fine:

screenshot

After I tried to browse through the code, but didn't catch the problem right away. I use the last version of aizynthfinder, taken from github.

Also, regarding this issue, I don't understand how max_transforms and number_of_steps are related. From the docs I've got, that max_transforms is the max depth of the MCTS tree, but why then number_of_steps is needed?

SGenheden commented 1 year ago

I believe this is an old bug that has to do with the incorrect comparison operator being used. In some places in the code it should be "<" but it is "<=" that is implemented. We might update this, but in principle it doesn't affect the algorithm. One just has to reimagine what "max_transforms" is.

What is the "number_of_steps" you are referring to?

tagirshin commented 1 year ago

For example, when I read hdf5 file with results and statistics of a tree search, I have these columns:

Screenshot

So here I don't understand why max_transforms and number_of_steps are different

P.S. sorry for the long answer

SGenheden commented 1 year ago

max_transforms refers to the depth of the MCTS search tree, i.e. it is a function over all routes extracted. number_of_steps refers to the depth of top-ranked route

I will create an issue to add proper documentation for the output dictionaries.

SGenheden commented 9 months ago

Fixed by #141