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

Gibbs Workflow -- Phonopy QHA refactor error #271

Open
mattmcdermott opened this issue Mar 7, 2019 · 10 comments
Open

Gibbs Workflow -- Phonopy QHA refactor error #271

mattmcdermott opened this issue Mar 7, 2019 · 10 comments
Labels
bug improvement reported issues that considered further improvement to atomate

Comments

@mattmcdermott
Copy link

mattmcdermott commented Mar 7, 2019

The phonopy QHA code was refactored in July 2018 and the variable "_max_t_index" no longer exists. It is currently used here:

max_t_index = phonopy_qha._qha._max_t_index
G = phonopy_qha.get_gibbs_temperature()[:max_t_index]
T = phonopy_qha._qha._temperatures[:max_t_index]

This needs to be updated for the current version of phonopy to be used with the Gibbs workflow (should be able to use _len instead).

@mkhorton
Copy link
Contributor

mkhorton commented Mar 7, 2019

As far as I can tell, the whole PhonopyQHA interface has been removed/simplified and replaced with just a QHA object, the method signatures look similar though. In other words, this now gives the ._qha object directly, and ._max_t_index has become ._t_max.

Not sure if there would be a better way of writing this analysis step without using 'protected' attributes?

@mattmcdermott
Copy link
Author

@mkhorton the PhonopyQHA interface is just for the API -- it has always just created a QHA object actually

@mkhorton
Copy link
Contributor

mkhorton commented Mar 7, 2019

Right, which is I guess why it was removed?

@mattmcdermott
Copy link
Author

It hasn't been removed though?

@mkhorton
Copy link
Contributor

mkhorton commented Mar 7, 2019

Ah my mistake, I must have been looking at an old commit.

@mattmcdermott
Copy link
Author

Haha it's alright, I thought the same thing. This is where it is defined FYI:

https://github.com/atztogo/phonopy/blob/b7cff9090ae17be41b209f6b89a6f49cd83c36ab/phonopy/api_qha.py#L38

@ehomer
Copy link

ehomer commented Oct 29, 2021

I am trying to run a QHA analysis with phonopy and just got this error. Do I have an old version of the code or is this still an issue?

@mattmcdermott
Copy link
Author

Hi @ehomer, sorry for the late reply. I think this bug has not been fixed actually -- I don't see any commits to the code since 2017.

I ran the Gibbs workflow so long ago I'm not actually sure what edits I made to get things to work... this was also at the beginning of me starting as a grad student so I didn't do a great job contribution-wise. If my old branch might be of any use, here it is:

https://github.com/mattmcdermott/atomate/tree/gibbs

It's possible I'll revisit if/when I do phonopy stuff again, but if you are able to make a pull request with the fix that would be much appreciated :)

@itsduowang itsduowang added bug improvement reported issues that considered further improvement to atomate labels Feb 9, 2022
@jmmshn
Copy link
Contributor

jmmshn commented Mar 9, 2022

Basic fix from PR #751
still need to check the results in detail and add tests.

@mattmcdermott
Copy link
Author

Thanks @jmmshn for addressing this!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug improvement reported issues that considered further improvement to atomate
Projects
None yet
Development

No branches or pull requests

5 participants