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

Combine atomate2 vasp calc types #764

Merged
merged 6 commits into from
Aug 21, 2023
Merged

Conversation

mjwen
Copy link
Member

@mjwen mjwen commented Jun 27, 2023

This PR addresses #735. The below has been performed:

  • add Molecular Dynamics task type
  • regenerate enums.py

TODO:

  • check the update in enums.py is OK, e.g. PB0->PBE0 and removing PBEsol (now using PBESol?).
  • use it from the atomate2 side to make sure everything works well
  • figure out what the line elif len([x for x in kpts.get("labels") or [] if x is not None]) > 0: in utils.py is doing and possibly rewrite it. (@utf can give a quick answer).

@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023

Codecov Report

Patch coverage: 97.61% and project coverage change: -0.27% ⚠️

Comparison is base (91ee6a4) 91.22% compared to head (0044ae3) 90.95%.
Report is 51 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #764      +/-   ##
==========================================
- Coverage   91.22%   90.95%   -0.27%     
==========================================
  Files         151      136      -15     
  Lines       11948    11723     -225     
==========================================
- Hits        10899    10663     -236     
- Misses       1049     1060      +11     
Files Changed Coverage Δ
emmet-core/emmet/core/vasp/calc_types/__init__.py 66.66% <ø> (-4.77%) ⬇️
emmet-core/emmet/core/vasp/calc_types/utils.py 82.53% <66.66%> (-4.35%) ⬇️
emmet-core/emmet/core/vasp/calc_types/enums.py 100.00% <100.00%> (ø)

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mjwen
Copy link
Member Author

mjwen commented Jun 30, 2023

  • For the 1st TODO, after I rerun generate.py, these changes happen. I guess they are supposed to happen and it is just we haven't run generate.py after modifying the other files?

  • For the 3rd TODO, the line is copied atomate2. I haven't no idea whether we need to add this line or not. If yes, can we simplify it?

Anyone knows these more please comment and we can then move forward.

@mjwen mjwen changed the title [WIP] Combine atomate2 vasp calc types Combine atomate2 vasp calc types Jun 30, 2023
@utf
Copy link
Member

utf commented Jul 19, 2023

figure out what the line elif len([x for x in kpts.get("labels") or [] if x is not None]) > 0: in utils.py is doing and possibly rewrite it.

I can't remember what this is doing. But I would be reluctant to remove it to avoid any breaking changes.

@mjwen
Copy link
Member Author

mjwen commented Jul 28, 2023

Ready for review!

@utf
Copy link
Member

utf commented Aug 16, 2023

Just checking if there has been any progress on this? Ideally we can get this merged to progress to a new atomate2 release.

@munrojm
Copy link
Member

munrojm commented Aug 16, 2023

Sorry for the delay, I will take a look today and test it myself today.

@munrojm munrojm merged commit 80b09f7 into materialsproject:main Aug 21, 2023
13 checks passed
@utf
Copy link
Member

utf commented Aug 22, 2023

Thanks @munrojm!


AM05 = "AM05"
GGA = "GGA"
PBE = "PBE"
PBESol = "PBESol"
PBEsol = "PBEsol"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjwen Why was this changed from PBEsol to PBESol? This (very minorly) breaks some of the validation code I'm working on. Also, it isn't consistent with the original PBEsol paper's capitalization (https://arxiv.org/abs/0711.0156)

@mjwen
Copy link
Member Author

mjwen commented Sep 1, 2023

Sorry that this causes trouble for you!

The change was introduced when merging this and the atomate2 enums.py, where PBESol.

I am OK to change it back, but we need to ensure this will not introduce any problem for atomate2.

@utf Any idea on this? Will atomate2 get into trouble if we use PEBsol? Or do you prefer PBESol?

@mjwen
Copy link
Member Author

mjwen commented Sep 1, 2023

Related, we also did PB0->PBE0. Not sure if this will cause trouble for others or not... @munrojm any thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants