Critic2Caller code bug fix (repost with a clearer title)

[pjf295]

Describe the bug Critic2Caller crashes due to incorrect array shape. A second bug generates incorrect input to critic2 resulting in a critic2 syntax error that aborts the run.

The corrected code is attached.

To Reproduce Any attempt to use Critic2Caller fails. This includes Critic2Caller.from_path() and Critic2Caller_from_chgcar()

1. Attempting to use Critic2Caller.from_path() or Critic2Caller_from_chgcar() both fail with an error due to improper array shape being given to numpy's linalg and trace methods (lines 418 and 407). These routines expect a 2D matrix and the field_hessian list is one dimensional.

   There is also a syntax error generated in the input code for critic2 regarding the zpsp statement. This should be on a line by itself in the input, not appended to the load command.

2. The chgcar files used are too big to post here, but any set of chgcars will fail due to the above reasons.

Expected behavior A Critic2Analysis object should be returned

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: version independent (but this was on Centos 7)
• Version "2022.11.7"

Additional context Add any other context about the problem here.

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

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

critic2_caller.py.zip

[mkhorton]

Thanks @pjf295, would you be willing to submit a small PR with the fix? To verify, which version of critic2 is this, so that we can indicate in the docstring? (This code did use to work with an earlier version.)

[pjf295]

Hi Matthew,

The patch is for the latest version of Critic2 downloaded December 8, 2022. Unfortunately, it seems that the author doesn’t make changes to the version number (now 1.1) very frequently. I must admit, I have not submitted a pull request before so I will have to look up the procedure. Are there any details I should know about the submission process (branch etc.?)

Best wishes, Paul Fons

Paul Fons @.***

Keio University, Faculty of Science and Technology, Department of Electronics and Electrical Engineering

〒223-8522 3-14-1 Hiyoshi, Kohoku-ku, Yokohama, Kanagawa 223-8522, Japan Keio University Faculty of Science and Technology Yagami Campus.

〒223-8522　横浜市港北区日吉3-14-1 慶應義塾大学理工学部電気情報工学科 23-315　

On Dec 21, 2022, at 8:11, Matthew Horton @.***> wrote:

Thanks @pjf295 https://github.com/pjf295, would you be willing to submit a small PR with the fix? To verify, which version of critic2 is this, so that we can indicate in the docstring? (This code did use to work with an earlier version.)

— Reply to this email directly, view it on GitHub https://github.com/materialsproject/pymatgen/issues/2779#issuecomment-1360436264, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQT5S3HDRXKL33ZU3R3BFNLWOI4J5ANCNFSM6AAAAAATD75CMQ. You are receiving this because you were mentioned.

[mkhorton]

For a small change like this, it's actually possible to do via the GitHub website -- for a first PR, this is a good procedure :)

To do this, you would navigate to https://github.com/materialsproject/pymatgen/blob/master/pymatgen/command_line/critic2_caller.py and click the edit button: https://github.com/materialsproject/pymatgen/edit/master/pymatgen/command_line/critic2_caller.py

From there, you can propose your change and the reasoning.

Typically, this would need to be accompanied with a test, but I can review your PR myself and make additional changes as appropriate.

[pjf295]

Hi Matthew,

Got it. I will submit a PR later today.

Best wishes, Paul

Paul Fons @.***

Keio University, Faculty of Science and Technology, Department of Electronics and Electrical Engineering

〒223-8522 3-14-1 Hiyoshi, Kohoku-ku, Yokohama, Kanagawa 223-8522, Japan Keio University Faculty of Science and Technology Yagami Campus.

〒223-8522　横浜市港北区日吉3-14-1 慶應義塾大学理工学部電気情報工学科 23-315　

On Dec 21, 2022, at 8:39, Matthew Horton @.***> wrote:

For a small change like this, it's actually possible to do via the GitHub website -- for a first PR, this is a good procedure :)

To do this, you would navigate to https://github.com/materialsproject/pymatgen/blob/master/pymatgen/command_line/critic2_caller.py https://github.com/materialsproject/pymatgen/blob/master/pymatgen/command_line/critic2_caller.py and click the edit button: https://github.com/materialsproject/pymatgen/edit/master/pymatgen/command_line/critic2_caller.py https://github.com/materialsproject/pymatgen/edit/master/pymatgen/command_line/critic2_caller.py From there, you can propose your change and the reasoning.

Typically, this would need to be accompanied with a test, but I can review your PR myself and make additional changes as appropriate.

— Reply to this email directly, view it on GitHub https://github.com/materialsproject/pymatgen/issues/2779#issuecomment-1360477859, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQT5S3BUXEY7LBVDSURSN2LWOI7RNANCNFSM6AAAAAATD75CMQ. You are receiving this because you were mentioned.