addressing issues 5, 8, (part of 13) - new

[erskordi]

No description provided.

[davebiagioni]

we can directly modify config variables in the run scripts, so mod.py is not needed

[erskordi]

ok, mod.py was moved to stable_radical_optimization folder and renamed to stable_rad_config.py. Loading mod.py was also removed from all scripts in alphazero folder

[pstjohn]

So one other issue: Node.reward is cached, so if we indeed use game.reset() (instead of the current new game per iteration) approach, the cached reward will be a ranked reward, which might no longer be valid. In general, I'm wondering if we want to separate node.reward from node.ranked_reward; but that might also involve changing game.mcts_step; where we propagate up leaf.reward values.

[pstjohn]

Does this work? It might be best practice to use a psycopg2.sql object:
https://www.psycopg.org/docs/sql.html

[pstjohn]

so the game table should be a record of recently played games, with their associated (real) rewards. gameid should also be a column

[pstjohn]

So this implies that anytime a terminal state is reached (which can happen often throughout a single game) this is being added to the 'game' table. I think ranked reward is something we only want to compute from recently played, final game states. So I imagine this code would go after adding to the replay buffer table

[erskordi]

OK, the

cur.execute("""
                INSERT INTO {table}_game
                (experiment_id, gameid, real_reward) 
                values (%s, %s);""".format(table=config.sql_basename), (
                    config.experiment_id, gameid, float(reward)))

is moved after adding to the replay buffer table. In order to distinguish between real and ranked rewards,
reward = game[-1].reward returns the real reward and later in the replay buffer we store the get_ranked_rewards(reward) ranked reward.

[pstjohn]

It doesn't look like you've pushed any changes yet, but my only concern with this approach is that, as written, game.py expects the node.reward value to be reward that we propagate up the MCTS tree; i.e., the ranked reward. So until we refactor that (#18), we should node.get_reward() return the ranked reward, and cache the intermediate true_reward somewhere like node._true_reward.

[erskordi]

Yes, I haven't pushed anything yet, I'll push asap, the responses to comments are mostly for me to check what I'm addressing

[erskordi]

Alright, I see what you mean here. I'm proceeding with it now

[pstjohn]

here is where I'd add the game (and final, real reward) to the game table. That does involve making sure that nodes cache their true reward value, rather than just their ranked reward.

[erskordi]

See comment above, let me know if this is a good strategy

[pstjohn]

See reply above. Just to be a bit more explicit; the node.value that gets estimated during MCTS should, for ranked rewards, lie between -1 and 1, and represent the probability that node is a win / loss by ranked rewards. And these are the values predicted by the policy network. But if we return the true reward in get_reward(), that calculation gets blasted with weird values (ie., 50) that are way outside that range

[davebiagioni]

unused, remove

[erskordi]

done

[davebiagioni]

unused, remove

[erskordi]

done

[davebiagioni]

unused, remove

[erskordi]

done

[davebiagioni]

here is where we can just modify config properties directly

[erskordi]

done

[davebiagioni]

from psj: Maybe we want to move these things over to a separate python script? Maybe initialize.py or similar.

then the submit_mcts.sh could do something like

python initialize.py  # Run the one time
srun python run_mcts.py  # Run on all workers

[erskordi]

Per our discussion, I created the stable_rad_config.py in directory stable_radical_optimization, where it overloads config.py and an initialize.py that creates PostgreSQL tables. I also updated submit_mcts.sh to execute:

python initialize.py
srun python run_mcts.py

[davebiagioni]

from psj: Otherwise, we'd want this not to drop tables, but CREATE TABLE IF NOT EXISTS

[erskordi]

Check comment above, use a separate script for initializing tables

[pstjohn]

can you push the new changes?

[erskordi]

OK, I'll push the updated scripts I have so far and continue working on the rest of the comments/changes

[davebiagioni]

psj: we should pull out the RR calculation into a separate function. For now, that could just live as a floating function in this file.

def get_ranked_rewards(reward, conn=None):

if conn is None:
    conn = psycopg2.connect(**dbparams)

with conn:
    ... do the calculation

[erskordi]

Created function get_ranked_rewards, do you think is better to put it on a separate script as well? Or keep it in run_mcts.py?

[pstjohn]

I'd keep it in run_mcts. We'll likely want it to eventually be a class method of Node

[erskordi]

OK, sounds good!

[davebiagioni]

this returns the actual reward, right? don't we want to return the ranked reward for MCTS?

[pstjohn]

Just to make sure: we'll need to re-calculate the ranked reward on the fly, it doesn't make sense to store ranked rewards in the reward buffer

[erskordi]

The values returned from the entire if-elif-else statement are all "filtered" through the get_ranked_rewards function to return ranked rewards instead of real rewards.

[davebiagioni]

we want ranked_reward here, too, right?

[pstjohn]

I'd disagree -- ranked rewards don't make much sense outside the context of the game they're evaluated in. If a later game discovers the same molecule, we'll want to make sure the ranked reward is current w.r.t the game buffer

[davebiagioni]

no you're right, i hadn't thought this out yet but per our teams chat i agree

[davebiagioni]

what about ranked_reward and final smiles?

[pstjohn]

More generally, any reason to assign these to variables rather than just doing something like

"CREATE TABLE {basename}_replay".format(basename=config.sql_basename)

[erskordi]

done

[pstjohn]

can we change this to a count?

[erskordi]

Yes, changed to

n_rewards = pd.read_sql_query("""
        select count(*) from {table}_reward
        """.format(table=config.sql_basename), conn)

[pstjohn]

sounds great! another option might be to try to put this logic into a single SQL query https://www.postgresql.org/docs/9.1/functions-conditional.html. That's a minor change though, let's leave that for a later optimization

[erskordi]

ok, let's keep it in mind for later : )

[pstjohn]

this probably won't work; i think you want it to read

import alphazero.config as config
import stable_rad_config

[erskordi]

Damn, you're right. And to think of it, I had it like that in the first place

[pstjohn]

the idea with the conn kwarg is that you likely already have a SQL connection open, so we might as well use it. no idea if the syntax i wrote will work though :)

[pstjohn]

Just adding some additional code comments via github. I think this is ready to merge now though

[pstjohn]

this seems to fix those retracing errors we were seeing

[pstjohn]

sometimes we won't want to erase our previous calculations -- I'm not sure this is the right solution long-term though

[pstjohn]

this removes those 'manual compile' warnings that weren't relevant

[pstjohn]

so this was a big issue -- the ranked rewards needs to be calculated based off recently played games; not recent rewards.

[erskordi]

I thought about that too, game holds the actual real rewards that we want to use for calculating r_alpha. Thanks!

[pstjohn]

same as above. Also note that you can use psycopg2 directly (rather than pandas.read_sql_query) for these single-value queries.

[erskordi]

Great, good to know!

[pstjohn]

I think this is what we want -- otherwise, in the initial game playing (before we've built up a minimum buffer of games) we'll choose invalid molecules rather than those that are just not optimal

[pstjohn]

so, this is an important point for future reward caching -- if two nodes calculate the same reward simultaneously, we can end up calculating a reward for a molecule already in the reward buffer. So we have to catch that conflict and 'do nothing' (i.e., don't add the duplicate)

[pstjohn]

I did an htop after sshing into one of the worker nodes: i don't think these games benefit from much threading. So we might as well just blast the nodes with a bunch of copies of these; this lets us run 36 concurrent games on each node.