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

Phonopy QHA fixed missing att #751

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

jmmshn
Copy link
Contributor

@jmmshn jmmshn commented Mar 5, 2022

Fixed attribute change related to Phonopy QHA change #271

I don't know the original version of phonopy that references the _max_t_index attribute but it appears they now use self._len in the same way:
Examples:
https://github.com/phonopy/phonopy/blob/ac7abb7e4bbbd3e45d76d21cf3e722176830a9e1/phonopy/qha/core.py#L258

https://github.com/phonopy/phonopy/blob/ac7abb7e4bbbd3e45d76d21cf3e722176830a9e1/phonopy/qha/core.py#L263

The source of the self._len is listed below and should be the same as the _max_t_index.

self._num_elems = len(self._temperatures)

https://github.com/phonopy/phonopy/blob/ac7abb7e4bbbd3e45d76d21cf3e722176830a9e1/phonopy/qha/core.py#L364


        beta = [0.0]
        for i in range(1, self._num_elems - 1):
            dt = self._temperatures[i + 1] - self._temperatures[i - 1]
            dv = self._equiv_volumes[i + 1] - self._equiv_volumes[i - 1]
            beta.append(dv / dt / self._equiv_volumes[i])

        self._thermal_expansions = beta

https://github.com/phonopy/phonopy/blob/ac7abb7e4bbbd3e45d76d21cf3e722176830a9e1/phonopy/qha/core.py#L1040


self._len = len(self._thermal_expansions)

https://github.com/phonopy/phonopy/blob/ac7abb7e4bbbd3e45d76d21cf3e722176830a9e1/phonopy/qha/core.py#L375

@jmmshn jmmshn changed the title fixed missing att Phonopy QHA fixed missing att Mar 5, 2022
bump maggma
@jmmshn
Copy link
Contributor Author

jmmshn commented Mar 8, 2022

Should be fixed after materialsproject/maggma#595 is merged.

@jmmshn
Copy link
Contributor Author

jmmshn commented Mar 9, 2022

@itsshawnzhang is it OK to add an extra requirements-optional.txt and pin the phonopy version?

@itsduowang
Copy link
Contributor

@itsshawnzhang is it OK to add an extra requirements-optional.txt and pin the phonopy version?

Yes of course. You can modify the requirements-optional and we then ca run through the tests. If everything works well, we can then move on to merge the modification to main branch.

@mkhorton
Copy link
Contributor

mkhorton commented Mar 9, 2022

I don't know the original version of phonopy that references the _max_t_index attribute but it appears they now use self._len in the same way

I think a bunch changed in phonopy 2? There's an old issue here discussing some API changes: #271 maybe?

@jmmshn
Copy link
Contributor Author

jmmshn commented Mar 9, 2022

@itsshawnzhang so I have an colleague that is going to test this. Can we get this small change merged then I promise to add the tests and additional docs once I have the actual test file?
I have "working" test files now --- as in the wf finished running
but I'm hoping to actually get a comparison of the results from someone that has done this "by hand" so we can confirm the wf works.

@mkhorton this PR is specifically addressing that issue, I looked at the new phonopy API a bit and I think the change gets the atomate code to do what it was originally designed to.

@mkhorton
Copy link
Contributor

mkhorton commented Mar 9, 2022

Ah, gotcha. Thanks @jmmshn. I think I ended up concluding it would be easier to write new workflows in atomate2, but I'm glad you've fixed this (I've been running phonon calculations manually)

@itsduowang
Copy link
Contributor

@itsshawnzhang so I have an colleague that is going to test this. Can we get this small change merged then I promise to add the tests and additional docs once I have the actual test file?

I have "working" test files now --- as in the wf finished running

but I'm hoping to actually get a comparison of the results from someone that has done this "by hand" so we can confirm the wf works.

@mkhorton this PR is specifically addressing that issue, I looked at the new phonopy API a bit and I think the change gets the atomate code to do what it was originally designed to.

Okay. I can help you with it. So just to make sure that the phonopy version requirement is in the optional requirements file. If it's not going to work comparably with new version of phonopy, we just need to open a new PR to change it back then.

Thanks for your contribution. Please let me know if there is any other problems or concerns. Cheers:)

@itsduowang itsduowang merged commit da9e6fa into hackingmaterials:main Mar 9, 2022
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

Successfully merging this pull request may close these issues.

3 participants