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

Fix bump phonopy #1006

Merged
merged 15 commits into from
Nov 30, 2024
Merged

Fix bump phonopy #1006

merged 15 commits into from
Nov 30, 2024

Conversation

naik-aakash
Copy link
Contributor

@naik-aakash naik-aakash commented Oct 4, 2024

Changes

  1. Update phonopy version
  2. Switch off the Gruneisen parameter computations at gamma point following (Gruneisen API results different when updating phonopy from v2.27.0 to >=v2.28  phonopy/phonopy#455 (comment))

Todo

  • Investigate what cause the change in gruneisen parameter value leading to failure in test
  • Fix failing phonon workflow with using latest phonopy version (v2.30.1)

@naik-aakash naik-aakash marked this pull request as draft October 4, 2024 11:13
@naik-aakash
Copy link
Contributor Author

Hi @JaGeo ,

I had a look at the phonopy source code changes between v2.28 and v.2.27, I could not really pinpoint what is causing the issue with different values of the average Gruneisen parameter here.

I guess it has something to do with following PR. But am not sure about it

phonopy/phonopy@c013a19

@JaGeo
Copy link
Member

JaGeo commented Oct 6, 2024

How large are the changes? Could you give more details? Thanks!

@naik-aakash
Copy link
Contributor Author

The average Gruneisen value now comes out to be 1.271535057337552 using v2.28 (using v2.27 we used to get 1.1882292157682082)

@JaGeo
Copy link
Member

JaGeo commented Oct 6, 2024

Yeah, I think we need a bit more analysis here. This is quite a large change

@naik-aakash
Copy link
Contributor Author

The latest version of phonopy causes phonon workflow test to also fail (cause of failure seems to originate from force constants computation default being changed)

@JaGeo
Copy link
Member

JaGeo commented Nov 18, 2024

I would need more details to act. E.g., How large is the change? Why does this only affect the Grüneisen parameter workflow? What is different?

We can do this privately if you prefer.

@naik-aakash
Copy link
Contributor Author

I would need more details to act. E.g., How large is the change? Why does this only affect the Grüneisen parameter workflow? What is different?

We can do this privately if you prefer.

As I have mentioned above, now with latest version phonon workflow test is also affected. I will try to share the details in next days

@JaGeo
Copy link
Member

JaGeo commented Nov 18, 2024

@naik-aakash i see! Thanks!

@naik-aakash naik-aakash changed the title [WIP] Fix bump phonopy Fix bump phonopy Nov 29, 2024
@naik-aakash naik-aakash marked this pull request as ready for review November 29, 2024 14:30
Copy link

codecov bot commented Nov 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.63%. Comparing base (4244da9) to head (abbc041).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
+ Coverage   72.82%   76.63%   +3.81%     
==========================================
  Files         187      187              
  Lines       13637    13627      -10     
  Branches     1370     1372       +2     
==========================================
+ Hits         9931    10443     +512     
+ Misses       3161     2626     -535     
- Partials      545      558      +13     
Files with missing lines Coverage Δ
src/atomate2/common/schemas/gruneisen.py 94.28% <100.00%> (+0.11%) ⬆️

... and 41 files with indirect coverage changes

@JaGeo
Copy link
Member

JaGeo commented Nov 29, 2024

@naik-aakash one last question: do you need all test files still or can one set be removed?

@naik-aakash
Copy link
Contributor Author

@naik-aakash one last question: do you need all test files still or can one set be removed?

Hi @JaGeo, Thanks for spotting this. I Have now removed the test files that are no longer used

@JaGeo JaGeo enabled auto-merge (squash) November 30, 2024 08:39
@JaGeo JaGeo merged commit 4bcf127 into materialsproject:main Nov 30, 2024
15 checks passed
@JaGeo
Copy link
Member

JaGeo commented Nov 30, 2024

@naik-aakash thanks. This looks good. I will merge.

@JaGeo
Copy link
Member

JaGeo commented Nov 30, 2024

For everyone else seeing this and wondering why we are changing tests:

By accident, one of the phonon tests used wrong test files. This only appeared as a problem when phonopy added more strict handling of the forces.

The Grüneisen parameter computation was also affected by a phonopy change. However, as this mostly affected an unrealiable value at gamma, we simply remove it from the mesh computation. We should have likely done this already before.

hrushikesh-s pushed a commit to leslie-zheng/atomate2 that referenced this pull request Dec 10, 2024
* bump phonopy version

* Update pyproject.toml

* replace bs plotter with pymatgen implementation > reduce code repetition

* replace bs plotter with pymatgen implementation > reduce code repetition

* switch off grunesien computation at gamma (unstable)

* fix assertions to correct expected derived properties

* fix failing test (use correct reference data)

* remove non used test_data, rename Si_phonons_4 > Si_phonons_3

* point to renamed ref test_data

---------

Co-authored-by: J. George <[email protected]>
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.

2 participants