Skip to content

Test#8

Open
Rodin1000 wants to merge 6 commits into
masterfrom
test
Open

Test#8
Rodin1000 wants to merge 6 commits into
masterfrom
test

Conversation

@Rodin1000
Copy link
Copy Markdown

No description provided.

@Rodin1000 Rodin1000 requested a review from kochkov92 April 26, 2019 15:56
Copy link
Copy Markdown
Contributor

@kochkov92 kochkov92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing some typos. I think having bonds_file is a good idea.
-) I suggest to stay compatible with the ED code on the wave function format at least for now.
-) I think rescaling at psi_target is more intuitive for someone who is reading the code.

Comment thread cgs_vmc/evaluation.py Outdated
config_ind = 0
for i in range(0, batch_size):
out_file.write('({},{})\n'.format(psi[i].real, psi[i].imag))
out_file.write('{} {}\n'.format(psi[i].real, psi[i].imag))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the purpose of testing vectors with python we can just use pandas to read-in files. In this case we maintain uniformity with ED code, which is a nice tool to test and compare against. Here's a possible way to read the current format:

def get_wf(file_name):
  df = pd.read_csv(file_name)
  df = df.apply(lambda col: col.apply(lambda val: complex(val.strip('()'))))
  return df.values[:, 0]

Comment thread cgs_vmc/evaluation.py Outdated
psi = session.run(wavefunction_value, feed_dict=feed_dict)
for i in range(0, config_ind):
out_file.write('({},{})\n'.format(psi[i].real, psi[i].imag))
out_file.write('{} {}\n'.format(psi[i].real, psi[i].imag))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as the above

Comment thread cgs_vmc/normalizer.py
for _ in range(normalization_iterations):
session.run(update_config)
session.run(update_norm_on_batch)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are not deploying this part yet, I would move it to prototypes; then we can recover this part of the code when needed.

Comment thread cgs_vmc/normalizer.py Outdated


def run_normalization_ops(
max_value: Any,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think max_value is probably a np.float or something.

Comment thread cgs_vmc/run_energy_evaluation.py Outdated
'Full path to the checkpoint directory.')

flags.DEFINE_string(
'bond_file', '',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: bonds_file
Name of the file that contains bonds of the Hamiltonian.

Comment thread cgs_vmc/run_training.py Outdated
'checkpoint_dir', '',
'Full path to the checkpoint directory.')
flags.DEFINE_string(
'bond_file', '',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for run_supervised_training.py

Comment thread cgs_vmc/training.py Outdated

psi = wavefunction(configs)
psi_target = target_wavefunction(configs)
psi_target = target_wavefunction(configs) * hparams.scale
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since LogOverlapSWO doesn't depend on the norm mismatch there is no reason to scale psi_target here.

Comment thread cgs_vmc/training.py Outdated

loss = tf.reduce_mean(
tf.squared_difference(psi, psi_target)
tf.squared_difference(psi, psi_target * hparams.scale)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move scaling up in the code to improve readability. One naturally would expect target to be target, not a target before scaling.

Comment thread cgs_vmc/training.py Outdated

loss = tf.reduce_mean(
tf.squared_difference(psi, psi_target * np.sqrt(2 ** n_sites)))
tf.squared_difference(psi, psi_target * hparams.scale))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here you also want to move the the scaling up, so that psi_target has the scale you want.

Comment thread cgs_vmc/training.py Outdated

loss = tf.reduce_mean(
tf.squared_difference(psi, psi_target * np.sqrt(2**n_sites)) /
tf.squared_difference(psi, psi_target * hparams.scale) /
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't thought about it before, but it makes more sense to put scale up to the psi_target, simply improves readability. (otherwise it's confusing that we are matching scaled target)

@Rodin1000 Rodin1000 requested a review from kochkov92 May 2, 2019 03:20
@Rodin1000
Copy link
Copy Markdown
Author

For normalizer.py, do you suggest to put the whole file into prototypes, or just keep the first function?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants