Some modification and new files for QHA #294
Conversation
There was a problem hiding this comment.
Hi @samarerstm, sorry for the delay in reviewing this PR.
I went through the code and left comments mainly on the first two files.
I did not go deep into the QHA_App because it is unclear to me how this should be integrated with the rest. Is the idea to keep it as an alternative and separate object with respect to the original AbstractQHA? Or do you plan to integrate it?
It seems that there is quite a bit of code duplication between QHA_App and AbstractQHA, so I was wondering if it wouldn't be possible to have different subclasses of the AbstractQHA to calculate different approximations (e.g. one subclass for E2Vib1, one for EinfVib1, and so on). More or less like now there are the QHA, QHA3PF and QHA3P subclasses to deal with other different approaches to the calculation. Or is this completely different?
Concerning the formatting of the code, I know that @gmatteo is not strict about PEP8 and similar coding conventions, but I think that it could be beneficial to pass the code through some tool like "black" to improve the formatting.
I should also add that I did not have the time to dig into the details of your paper, so I did check anything about the math or the formulas.
abipy/dfpt/deformation_utils.py
Outdated
|
|
||
|
|
||
| def generate_deformations_volumic(structure, eps_V=0.02, scales=[-1, 0, 1, 2, 3]): | ||
| spgrp = AbinitSpaceGroup.from_structure(structure) |
There was a problem hiding this comment.
it seems that here the spgrp is never used
abipy/dfpt/deformation_utils.py
Outdated
| structure2.lattice = Lattice(rprim2) | ||
| structures_new[formatted_namei] = structure2 | ||
| print(formatted_namei) | ||
| print(rprim2) |
There was a problem hiding this comment.
Remove the print statements?
There was a problem hiding this comment.
They are removed
| return structures_new | ||
|
|
||
| def generate_deformations(structure , eps=0.005): | ||
| spgrp = AbinitSpaceGroup.from_structure(structure ) |
There was a problem hiding this comment.
I have a question about this point: is it relevant that the spacegroup obtained here is guaranteed to match 100% the one in abinit? In that case it would be better to use AbinitInput.abiget_spacegroup, since AbinitSpaceGroup.from_structure relies on the SpacegroupAnalyzer`. If instead it is just important that the result is consistent throught the code, this should be fine.
In case, this may apply to all the cases where AbinitSpacegroup.from_structure is used
There was a problem hiding this comment.
I attempt to use AbinitInput.abiget_spacegroup, but I have not succeed . could you please change it as your prefer ?
| formatted_namei = f"{namei:04d}" | ||
|
|
||
| structure2 = structure.copy() | ||
| structure2.lattice = Lattice(rprim2) |
There was a problem hiding this comment.
If you want I think you can simplify the code by using internal pyamtgen functions.
structure2 = structure.copy()
structure2.scale_lattice(scopy.volume*(1.00 + eps_V * i))should lead to the same result.
There was a problem hiding this comment.
This new command does not work.
abipy/dfpt/qha.py
Outdated
|
|
||
| dt = f.temp[1] - f.temp[0] | ||
| alpha = (f.min_vol[2:] - f.min_vol[:-2]) / (2 * dt) / f.min_vol[1:-1] | ||
| if (self.eos_name == 'vinet') : |
There was a problem hiding this comment.
This is the more accurate version of the thermal expansion discussed in the paper, right?
But why does this change only for Vinet? Is it because it would need to be formulated for the other EOS as well?
I think this might be confusing, as one would get a more accurate approximation depending on the chosen EOS, but this is not clearly mentioned. Also, one would not be able to get the standard QHA approximation with the EOS fit.
Maybe a good solution could be to add an additional argument to the method. If True the method will calculate the thermal expansion coefficient with this approximation, if False using the original one. Also if True and self.eos is not Vinet raise an error. Would this make sense?
There was a problem hiding this comment.
Now this method will be work for
eos_list = [ 'taylor', 'murnaghan', 'birch', 'birch_murnaghan','pourier_tarantola', 'vinet', 'antonschmidt']
there will be 2 other eos "deltafactor" which is smooth itself and do not need this method and
'numerical_eos' which does not work correctly for QHA method. most of my test with 'numerical_eos' failed and this eos can not interpolate the energies for my case.
abipy/dfpt/qha.py
Outdated
| alpha_qha_b = np.zeros( num-2) | ||
| alpha_qha_c = np.zeros( num-2) | ||
| if (tref==None): | ||
| alpha_qha_a = (aa_qha[2:] - aa_qha[:-2]) / (2 * dt) / aa_qha[1:-1] |
There was a problem hiding this comment.
at variance with what happens in get_thermal_expansion_coeff, here the coefficient is calculated with just the expression, even if the EOS is Vinet. Doesn't this mean that the result of the plots of plot_thermal_expansion_coeff and plot_thermal_expansion_coeff_abc will be inconsistent?
There was a problem hiding this comment.
In new version plot_thermal_expansion_coeff_abc is in inconsistent with plot_thermal_expansion_coeff
| from abipy.dfpt.gruneisen import GrunsNcFile | ||
|
|
||
|
|
||
| class QHA_App(metaclass=abc.ABCMeta): |
There was a problem hiding this comment.
Standard convention is to name classes with camel case style.
Also, why is this an abstract class?
There was a problem hiding this comment.
I need to discuss this file with Matteo. He asked me to submit a merge request so he can review it.
Some modification on qha.py
New file for v-ZSISA-QHA approximation method ["Physical Review B 110 (1), 014103"]
New file for generating necessary deformation needed for ZSISA-QHA method