Skip to content

Conversation

@Rodin1000
Copy link

No description provided.

@Rodin1000 Rodin1000 requested a review from kochkov92 April 26, 2019 15:56
Copy link
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.

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
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]

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
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

for _ in range(normalization_iterations):
session.run(update_config)
session.run(update_norm_on_batch)

Copy link
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.



def run_normalization_ops(
max_value: Any,
Copy link
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.

'Full path to the checkpoint directory.')

flags.DEFINE_string(
'bond_file', '',
Copy link
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.

'checkpoint_dir', '',
'Full path to the checkpoint directory.')
flags.DEFINE_string(
'bond_file', '',
Copy link
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


psi = wavefunction(configs)
psi_target = target_wavefunction(configs)
psi_target = target_wavefunction(configs) * hparams.scale
Copy link
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.


loss = tf.reduce_mean(
tf.squared_difference(psi, psi_target)
tf.squared_difference(psi, psi_target * hparams.scale)
Copy link
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.


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
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.


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
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
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.

3 participants