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 phonon + Lobster flow by removing magmoms before passing to phonopy #751

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

naik-aakash
Copy link
Contributor

@naik-aakash naik-aakash commented Feb 27, 2024

Changes

  1. Update pymatgen, mp-api and Lobsterpy version
  2. Add checks to remove magmom from structure to avoid phonopy failures

Closes #739

@naik-aakash naik-aakash changed the title Fix phonon flow [WIP] Fix phonon flow Feb 27, 2024
@naik-aakash
Copy link
Contributor Author

Hi @JaGeo and @utf , this changes should in principle fix the phonon workflow once mp-api is compatible with the latest pymatgen version.

Tests fail due to dependencies conflicts

@naik-aakash naik-aakash changed the title [WIP] Fix phonon flow Fix phonon + Lobster flow Feb 28, 2024
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.22%. Comparing base (62ae659) to head (031521d).
Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
+ Coverage   76.19%   76.22%   +0.02%     
==========================================
  Files          87       88       +1     
  Lines        7178     7191      +13     
  Branches     1057     1051       -6     
==========================================
+ Hits         5469     5481      +12     
- Misses       1388     1393       +5     
+ Partials      321      317       -4     
Files Coverage Δ
src/atomate2/common/jobs/phonons.py 86.20% <100.00%> (+1.50%) ⬆️

@naik-aakash
Copy link
Contributor Author

Hi @utf and @JaGeo , this PR could be merged now if there are no comments on it 😄

@JaGeo
Copy link
Member

JaGeo commented Feb 28, 2024

@naik-aakash I am not sure removing the initial magnetic moments in all cases is a good solution. We should rather disenable the seekpath option implemented in Phonopy in that case.

@JaGeo
Copy link
Member

JaGeo commented Feb 28, 2024

Here is a discussion about it: materialsproject/pymatgen#3555

@naik-aakash naik-aakash changed the title Fix phonon + Lobster flow [WIP] Fix phonon + Lobster flow Feb 28, 2024
@naik-aakash
Copy link
Contributor Author

Here is a discussion about it: materialsproject/pymatgen#3555

Ohh, ok, I see. I will check it. Thanks! 😃

@JaGeo
Copy link
Member

JaGeo commented Feb 28, 2024

Easy solution for now: removing all magnetic moments and adding a warning that we currently do not consider the initial magnetic structure for determining the symmetry and adding this also clearly in the documentation.

@utf what do you think? In this way, we could get a new version out very fast.

@JaGeo
Copy link
Member

JaGeo commented Feb 28, 2024

As I have not run magnetic structures with this workflow so far, I would like to do some tests if we implement this, add some additional features (manually setting the primitive cell, skipping band structure or options to set the kpath manually)

@utf
Copy link
Member

utf commented Feb 28, 2024

Easy solution for now: removing all magnetic moments and adding a warning that we currently do not consider the initial magnetic structure for determining the symmetry and adding this also clearly in the documentation.

Yes, this is a good solution.

@naik-aakash
Copy link
Contributor Author

Thanks @utf and @JaGeo , I will now add a warning and also update this in documentation of phonon workflow.

@JaGeo
Copy link
Member

JaGeo commented Feb 28, 2024

Tagging @JonathanSchmidt1 in case he might want to contribute to atomate2 with his implementation of a magnetic phonon workflow in a follow-up PR.

@JonathanSchmidt1
Copy link
Contributor

It might take a few weeks till I find the time but I would be happy to add it.
It will just not have the full functionality of the normal phonon workflow as
PRIMITIVE_AXES = auto and BAND = auto are not allowed with MAGMOM in phonopy.

@JaGeo
Copy link
Member

JaGeo commented Feb 28, 2024

@JonathanSchmidt1 Great. Yes, sure. I have mentioned some suggestions above as well

@utf
Copy link
Member

utf commented Feb 28, 2024

Thanks @naik-aakash and @JaGeo

@utf utf merged commit 5055030 into materialsproject:main Feb 28, 2024
7 checks passed
@janosh janosh changed the title [WIP] Fix phonon + Lobster flow Fix phonon + Lobster flow by removing magmoms before passing to phonopy Feb 28, 2024
@utf utf added the fix Bug fix PR label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New pymatgen breaks lobster and phonopy workflows
4 participants