SSCHAcode / python-sscha

The python implementation of the Stochastic Self-Consistent Harmonic Approximation (SSCHA).
GNU General Public License v3.0
55 stars 21 forks source link

Ensemble.save() does not save forces, energies and stresses #126

Closed DjordjeDangic closed 1 year ago

DjordjeDangic commented 1 year ago

If we are using the compute_ensemble() function to calculate forces, energies, and stresses and afterward use ensemble.save() function, it does not save forces, stresses, or energies because ensemble.force_computed is False. This should be updated in the compute_ensemble() function I guess. Also, maybe I nice functionality would be to pass a number of configurations variable in the ensemble.load_bin() function like it is in the ensemble.load() function. I find it useful when converging calculations.

diegomartinez2 commented 1 year ago

In 'ensemble.load_bin()' the number of configurations are calculated from the length of the energies as I can see in the code (taken from the numpy-formated file). After the 'ensemble.load_bin()' is called there should be a 'ensemble.N' variable with the number of configurations. Meanwhile in the 'ensemble.load()' function it needs the variable because we create the arrays before we fill them with the file's data.

diegomartinez2 commented 1 year ago

I'm looking at the "ensemble.force_computed is False" problem. By using 'compute_ensemble()' I the "force_computed" is changed when the 'get_energy_force()' is called; but when using the cluster, the function 'cluster.compute_ensemble()' is called that do not have "force_computed" calls. So the problem looks like is located in the cluster part of the code.

mesonepigreco commented 1 year ago

Ok then definitively this is a bug (luckily an easy one to fix). Right now to converge we can use the split and merge function of the ensemble. However, I do understand that they are quite non intuitive to do something as simple as limit the number of configurations. I agree with Diego that introducing N in the load_bin is not the best thing, as it could be easy to specify the wrong number by mistake and accidentally use less configurations, especially because the code needs anyway to load all the configurations in the binary format (unlike with the standard file format that the extra configurations are not loaded at all).

If you agree, we can add a simpler function, something like reduce_to(N_cfgs : int) to remove from the ensemble the extra configurations and do the convergence test in an easier way.

DjordjeDangic commented 1 year ago

Great! Glad it's resolved!