MolecularAI / aizynthfinder

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

UCB formula question #126

Open willfinnigan opened 1 year ago

willfinnigan commented 1 year ago

Hey Samual and team,

Thanks for this awesome work.

Just a quick question about your UCB formula.

I've seen the UCB1 formula written as wi/ni + c*sqrt(ln(N)/ni).

But in your code you do c*sqrt(2*ln(N)/ni) for the exploration part.

I believe you have C set to 1.4 (ie approximately sqrt(2)). Are you setting C twice? - once hard coded into the UCB formula with the sqrt(2*ln...), and once with the C in the config? Or perhaps I am confused about the UCB formula?

Kind regards, Will

SGenheden commented 1 year ago

Hello Will,

Thanks for this. The formula implemented is of course from Thakkar et al. Chem Sci (2020) and I don't think that anyone has looked at this equation in 3 years. So thanks for keeping us on our toes :-) At some point in time, I did convince myself that this was correct, but I am unable to do this today. It very much seems like we include the typical value of C twice. I don't necessarily thinks it is anything wrong with using a different value of C than sqrt(2), and we haven't seen much effect of changing C in the past (although I would like to re-do those tests). So in short: consider the AiZynthFinder to me an atypical implementation of MCTS (it already is :-) in other respects). I don't see that we will change this anytime soon.

BR Samuel