materialsproject / pymatgen

Python Materials Genomics (pymatgen) is a robust materials analysis code that defines classes for structures and molecules with support for many electronic structure codes. It powers the Materials Project.
https://pymatgen.org
Other
1.52k stars 864 forks source link

Critic2Caller generates incorrect input in script #2770

Open pjf295 opened 1 year ago

pjf295 commented 1 year ago

Describe the bug The Critic2Caller.from_path method should read in the charge densities from a Vasp calculation (AECCAR0 and AECCAR2) to generate the a total electron density field. The script the runs the external critic2 program generates a syntax error and critic2 fails resulting in a failure. In particular, the comand zpsp (that defines the core contribution for an atom by setting the pseudopotential charge) must be executed on a line by itself. The script incorrectly places the load command and the zpsp command on the same line

For my example. The script generates:

load int.CHGCAR id chg_int zpsp C 4.0 N 5.0

leading to the syntax error (and failure of critic2)

ERROR : load int.CHGCAR id chg_int zpsp C 4.0 N 5.0 ERROR (load): wrong syntax in ZPSP

The command should read

load int.CHGCAR id chg_int zpsp C 4.0 N 5.0

To Reproduce The files are too large to post, but the above comments should allow the issue to be reproduced.

Provide any example files that are needed to reproduce the error, especially if the bug pertains to parsing of a file.

Expected behavior The critic2 information should be returned to the caller for further processing. Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

Additional context The source code from critic2caller (below) adds the zpsp_str to the end of the load CHGCAR command . I believe the last line input_script should read

input_script += [zpsp_str]

Original code for reference

    # Load data to use for analysis
    if chgcar:
        input_script += ["load int.CHGCAR id chg_int", "integrable chg_int"]
        if zpsp:
            zpsp_str = " zpsp " + " ".join([f"{symbol} {zval}" for symbol, zval in zpsp.items()])
            input_script[-2] += zpsp_str
pjf295 commented 1 year ago

I realized I made a type above. The command currently generated takes the form: load int.CHGCAR id chg_int zpsp C 4.0 N 5.0 where C and N are the total charges of the PAW pseudopotentials used in this example. This statement is flagged as an error by critic2. Perhaps what the authors intended was load int.CHGCAR id chg_int core zpsp C 4.0 N 5.0 or simply moving the zpsp statement to the next line also works:

load int.CHGCAR id chg_int 
zpsp C 4.0 N 5.0
pjf295 commented 1 year ago

I have now fixed the code in critic2caller.py so that structuregraphs can be generated. There were two problems.

  1. The zpsp command in the original code generated an error in critic two. Moving the zpsp command to a new input line solved the problem.
  2. The code uses linear algebra routines to determine the eigenvalues and eigenvectors of the field_hessian. The linear algebra routines require a 2D matrix as input. The original code in critic2caller stored the field_hessian in a one dimensional list. Using numpy's reshape function to format the field_hessian as a 3x3 matrix fixes the error. A diff of the content is show below.
197c197
<                 input_script[-2] += zpsp_str
---
>                 input_script += [zpsp_str]
272a273
> 
390c391
<         self.field_hessian = field_hessian
---
>         self.field_hessian = np.array(field_hessian).reshape([3,3])
464a466
> 
499a502
> 
657c660
<                 field_hessian=p["hessian"],
---
>                 field_hessian=np.array(p["hessian"]).reshape([3,3]),
681a685
> 
840c844
<                 unique_critical_points[unique_idx].field_hessian = hessian
---
>                 unique_critical_points[unique_idx].field_hessian = np.array(hessian).reshape([3,3])
janosh commented 1 year ago

Sorry for the very late reply. We'd be happy to take PR that fixes this, especially if it includes additional tests.