microsoft / syntheseus

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

Rename value function to search heuristic #70

Closed AustinT closed 7 months ago

AustinT commented 7 months ago

Retro*, MCTS, and proof number search all use some kind of function to estimate the "potential" of leaf nodes in the search graph and choose which node to expand at each iteration. In syntheseus we called this the "value function". However, in hindsight I think this is a poor name because:

  1. Value function has a very specific meaning in reinforcement learning: it is the expected reward obtained from following a policy and none of the uses in syntheseus fit that. At best they are estimates of the value function.
  2. From the RL setting, value function has the connotation of "higher is better", but for algorithms like retro* lower values are better. This is probably confusing to users.

To improve clarity, this PR renames value function to "search heuristic" whenever referring to a "general" value function rather than something which is specifically a value function (e.g. in MCTS). I think the term "search heuristic" is both more accurate and less confusing than value function, and at least to me the term implies that it will be interpreted differently for different algorithms.

AustinT commented 7 months ago

Let's leave "value function" as it is. I also don't think it is confusing. We can add a comment or documentation to clarify that for some algorithms the value function is a (heuristic) negative cost function

Do you mean it is not a confusing term in general or just for the specific MCTS example which Kris raised above?

If you think "value function" is better than "search heuristic" in general then let's not change it; it is just a name. In that case I can close this PR.

AustinT commented 7 months ago

Ok let's not merge this. I will close the PR.