Skip to content

Add support for MTKNPT - #89

Merged
shyuep merged 15 commits into
materialyzeai:mainfrom
Andrew-S-Rosen:patch-1
Aug 7, 2025
Merged

Add support for MTKNPT#89
shyuep merged 15 commits into
materialyzeai:mainfrom
Andrew-S-Rosen:patch-1

Conversation

@Andrew-S-Rosen

@Andrew-S-Rosen Andrew-S-Rosen commented Aug 5, 2025

Copy link
Copy Markdown
Contributor

This is a follow-up to #88 to add support for MTKNPT. This is the newly recommended method for NPT simulations in ASE, with NPT not being recommended due to stability issues (see https://gitlab.com/ase/ase/-/merge_requests/3765). In #88, I only added the isotropic version. I didn't see the regular MTKNPT until now.

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

Andrew-S-Rosen and others added 3 commits August 5, 2025 19:51
This is a follow-up to materialyzeai#88 to add support for `MTKNPT`. This is the newly recommended method for NPT simulations in ASE, with `npt` being deprecated (see https://gitlab.com/ase/ase/-/merge_requests/3765). In materialyzeai#88, I only added the isotropic version. I didn't see the regular `MTKNPT` until now.

Signed-off-by: Andrew S. Rosen <asrosen93@gmail.com>
Signed-off-by: Andrew S. Rosen <asrosen93@gmail.com>
@Andrew-S-Rosen Andrew-S-Rosen changed the title Add support for MTKNPT [WIP] Add support for MTKNPT Aug 5, 2025
@Andrew-S-Rosen

Copy link
Copy Markdown
Contributor Author

It looks like we are blocked by ASE. The MTKNPT class is only available in master and not the most recent PyPI release. We will have to wait to merge this until the next ASE release.

Signed-off-by: Andrew S. Rosen <asrosen93@gmail.com>
@Andrew-S-Rosen

Andrew-S-Rosen commented Aug 6, 2025

Copy link
Copy Markdown
Contributor Author

@Naisargi-Goyal, since it will take some time for a new ASE version to be released on PyPI (which is needed to merge this PR), please post here if you find anything unusual or undesirable about the implementation I made or if you would like any improvements to be made. I encourage you to check that the default parameters I used are sensible and such.

@shyuep

shyuep commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

You can always install ASE in the CI using the main branch of the ASE repo.

@Andrew-S-Rosen

Copy link
Copy Markdown
Contributor Author

@shyuep If I do that, I would need to ensure the import doesn't cause any breakage for downstream users who have not updated to the master branch of ASE. I can guard the import with a try/except or an ASE version check. If that sounds okay with you, I am happy to proceed.

@shyuep

shyuep commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

Yes. But think it is up to you. If you feel it is not an urgent feature, we can always wait for new ASE to be released rather than doing all these hoops.

Comment thread pyproject.toml
]
dependencies = [
"ase>=3.23.0",
"ase>=3.25.0",

@Andrew-S-Rosen Andrew-S-Rosen Aug 6, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In hindsight, this should have been added in #88. I have added it now. Note that the master branch of ASE is 3.26.0b1.

@Andrew-S-Rosen Andrew-S-Rosen changed the title [WIP] Add support for MTKNPT Add support for MTKNPT Aug 6, 2025
Comment thread tests/test_md.py
Comment on lines +21 to +23
@pytest.fixture(scope="module", autouse=True)
def set_seed() -> None:
np.random.seed(42) # noqa: NPY002

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was getting arbitrary test failures between my local machine and CI due to differences in the random initializations. I have set a seed in the test suite to avoid this from happening in the future, although this also involves changes to the test values. I have confirmed that this PR does not break any behavior by ensuring that the test values remain unchanged with the main branch of matcalc as well as with ASE 3.23.0, 3.24.0, and 3.25.0.

@codecov

codecov Bot commented Aug 6, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.22%. Comparing base (18ac909) to head (9a44a18).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/matcalc/_md.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #89   +/-   ##
=======================================
  Coverage   96.22%   96.22%           
=======================================
  Files          22       22           
  Lines        1167     1167           
=======================================
  Hits         1123     1123           
  Misses         44       44           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Andrew-S-Rosen

Copy link
Copy Markdown
Contributor Author

@shyuep: This is ready for your review. The 1 missing line of coverage is for the scenario where the ASE version is < 3.26.0, which I have confirmed works fine locally.

shyuep and others added 6 commits August 6, 2025 15:13
Signed-off-by: Shyue Ping Ong <shyuep@users.noreply.github.com>
Signed-off-by: Andrew S. Rosen <asrosen93@gmail.com>
@shyuep
shyuep merged commit 1e2433d into materialyzeai:main Aug 7, 2025
1 of 5 checks passed
@shyuep

shyuep commented Aug 7, 2025

Copy link
Copy Markdown
Contributor

Thanks.

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