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

CP2K Support #157

Merged
merged 88 commits into from
Mar 13, 2023
Merged

CP2K Support #157

merged 88 commits into from
Mar 13, 2023

Conversation

nwinner
Copy link
Contributor

@nwinner nwinner commented Jul 19, 2022

Summary

This PR is for a WIP integration of CP2K support for atomate2. Defects will be included in a different PR.

Additional dependencies introduced (if any)

  • New CP2K features that will be in the main pymatgen by the time this is changed from WIP to production ready.

TODO (if any)

  • Sets
  • Basic Jobs
  • Basic Flows including hybrid flows
  • Basic schemas/calc types
  • Basic tests.

Basic functionality for CP2K atomate2 behavior: schemas, sets, jobs, flows, powerups, files, drones, and some basic tests are all included.
Changed NonSCF to bypass the validator and handler. Otherwise it will complain about non convergence. Removed convergence test in TaskDoc, but kept the check that cp2k completed running.
Moved to another branch
@nwinner nwinner changed the title [WIP] CP2K Support CP2K Support Oct 14, 2022
Copy link
Member

@utf utf left a comment

Choose a reason for hiding this comment

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

Thanks @nwinner. That looks like a good solution. I think we're ok to merge once tests and linting passes.

@utf
Copy link
Member

utf commented Mar 7, 2023

@janosh I see codespell is flagging some issues in non-python or md files. Is it possible to only include md and py files in the linting? I tried to figure out how to do it but couldn't work it out.

@janosh
Copy link
Member

janosh commented Mar 7, 2023

You could use pre-commit's types_or:

- repo: https://github.com/codespell-project/codespell
  rev: v2.2.2
  hooks:
    - id: codespell
      types_or: [python, rst, markdown, yaml, toml, ...]

@utf
Copy link
Member

utf commented Mar 7, 2023

That's really helpful! Thanks.

@utf
Copy link
Member

utf commented Mar 7, 2023

@nwinner I've pushed the codespell fix to main so if you merge that in it should fix those errors at least.

@utf
Copy link
Member

utf commented Mar 7, 2023

@nwinner you might need to set up the package data in pyproject.toml to fix the other failing tests:
See this section:

[tool.setuptools.package-data]
atomate2 = ["py.typed"]
"atomate2.vasp.sets" = ["*.yaml"]
"atomate2.vasp.schemas.calc_types" = ["*.yaml"]

You can also set up the pre-commit to run automatically by running:

pre-commit install

By running

pre-commit run --all

It will run on all your files and in some cases fix them.

@nwinner
Copy link
Contributor Author

nwinner commented Mar 7, 2023

Thanks @utf. I've merged the new master. Not sure why some linting is still failing though. I have the latest mypy on my laptop running pre-commit, and yet the github wf says mypy is failing. I'll investigate.

@nwinner
Copy link
Contributor Author

nwinner commented Mar 8, 2023

So I found some issues.

The pymatgen version in the config file for atomate2 is before the cp2k 2.0 updates, which is in 2023.1.9 release and beyond. That's causing some test errors.

Bigger issue is Custodian's latest release is May 25, 2022, and the compatibility with cp2k 2.0 was released in December 2022. So there's no release to reference in the config file. Can the requirements link to a github version, or do I have to have shyue release a new version of custodian?

@utf
Copy link
Member

utf commented Mar 8, 2023

Ok, we can update the pymatgen version and we should ask to get another custodian release. I’d rather not use the GitHub custodian.

@nwinner
Copy link
Contributor Author

nwinner commented Mar 8, 2023

I've emailed Shyue about when/if he can do a new custodian release. I'll wait to hear back from him.

@nwinner
Copy link
Contributor Author

nwinner commented Mar 13, 2023

@utf We got a new Custodian release. Allowed me to fix a few things with tests. So this is ready to merge unless you have any more comments.

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #157 (35ba06c) into main (b620471) will increase coverage by 2.34%.
The diff coverage is 84.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #157      +/-   ##
==========================================
+ Coverage   76.90%   79.25%   +2.34%     
==========================================
  Files          62       78      +16     
  Lines        5317     7688    +2371     
  Branches      755      987     +232     
==========================================
+ Hits         4089     6093    +2004     
- Misses       1020     1302     +282     
- Partials      208      293      +85     
Impacted Files Coverage Δ
src/atomate2/cp2k/schemas/calc_types/_generate.py 0.00% <0.00%> (ø)
src/atomate2/cp2k/schemas/calc_types/utils.py 50.00% <50.00%> (ø)
src/atomate2/cp2k/run.py 53.33% <53.33%> (ø)
src/atomate2/cp2k/drones.py 55.17% <55.17%> (ø)
src/atomate2/cp2k/flows/core.py 61.79% <61.79%> (ø)
src/atomate2/cp2k/sets/base.py 63.84% <63.84%> (ø)
src/atomate2/cp2k/schemas/calculation.py 70.31% <70.31%> (ø)
src/atomate2/cp2k/files.py 76.11% <76.11%> (ø)
src/atomate2/common/utils.py 82.14% <82.14%> (ø)
src/atomate2/cp2k/schemas/task.py 82.88% <82.88%> (ø)
... and 8 more

... and 1 file with indirect coverage changes

Copy link
Member

@utf utf left a comment

Choose a reason for hiding this comment

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

This looks fantastic! Very excited to have this in atomate2. Thanks for all the work with the tests, they look perfect.

@utf utf merged commit 7c0f27b into materialsproject:main Mar 13, 2023
@utf utf added the feature A new feature being added label Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature being added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants