Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Critically slow loop in _dipole_correction_init #277

Open
tyst3273 opened this issue Jun 16, 2023 · 5 comments
Open

Critically slow loop in _dipole_correction_init #277

tyst3273 opened this issue Jun 16, 2023 · 5 comments

Comments

@tyst3273
Copy link

I don't guess this is a bug, but the loop over 'max_shells' in ForceConstants._dipole_correction_init() is so slow as to be unusable ... is something wrong, or is this expected behavior? I have absolutely no idea whats going inside this part of the code, so I can't try to diagnose it myself! Sorry for my ignorance. (NOTE: you have to use the corrected version of the phonopy.py reader file, see the other issue: #276

Additional info: My calculation has 1296 atoms. It is from a DFPT calculation with Abinit that I converted into Phonopy force constants using Abipy. The data are read using the 'from_phonopy' method.

The dispersions from Phonopy take several seconds to calculate and are as expected.

See the attachment for the working example. I am using v1.2.0 installed with conda.

test.zip

@ajjackson
Copy link
Collaborator

Hi Ty,

I also found slow performance on this for a large system recently and discussed with @krefson . It looks like there is some room for optimisation!

@tyst3273
Copy link
Author

Thanks for the reply. An update: it looks like _dipole_correction_init() is accidentally(?) called twice. Once when the data are read from phonopy and then again when calculate_qpoint_phonon_modes() is called.

On line 1785 in force_constants.py, _dipole_correction_init is called but is assigned to a local variable dipole_data.
Later, on line 577, it is checked if ForceConstants has the attribute _dipole_init_data. Since it doesn't (its not defined anywhere else), _dipole_correction_init is called again.

Should _dipole_init_data be assigned the first time _dipole_correction_init is called? Since this methods takes so much time, I am trying to avoid calling it twice by mistake!

Thanks!

@ajjackson
Copy link
Collaborator

ajjackson commented Jun 19, 2023

I'd have to check more closely to see if something is going wrong, but there is a logical reason it would be called twice in this situation: Phonopy uses a different force constants convention (in common with some other codes including Quantum Espresso) from Euphonic (which shares its convention with CASTEP).

In the first convention the long-range contribution is left in the force constants; at the point of performing Fourier interpolation, the long-range components are calculated and subtracted to obtain the "short-ranged" force constants.

In the second convention, the subtraction takes place before writing/storing/exchanging force constants.

So when Euphonic imports force constants from Phonopy, it needs to compute the long-ranged part and subtract to obtain the short-ranged force constants that make a valid, serialisable ForceConstants object. You might write it to a JSON file at this point, so it can't be deferred to later. (Well, except by clever lazy-evaluation magic... but conceptually it is already short-range data.)

Then when we go to perform Fourier interpolation, the long-range dipole model is needed to get the correct dispersion.

That said, It does seem like a good idea to cache the _dipole_correction_init() data on import so that if one does import and then interpolate the data in one Python session (a common use-case) the second init can be avoided. Indeed, there is even an attribute to store it...

@krefson
Copy link

krefson commented Jun 21, 2023 via email

@ajjackson
Copy link
Collaborator

That does look very interesting. A few potential hazards:

  • the Python bindings don't seem to be documented
  • epsilon seems to be restricted to a scalar float
  • not immediately clear if/which methods can provide the second-derivatives we're after; docs indicated that we get energy and sometimes "virials"
  • only a few methods support non-cubic geometry

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

No branches or pull requests

3 participants