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

Move occ_tol to init in OrderDisorderedStructureTransformation #4274

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Tinaatucsd
Copy link
Contributor

Summary

Major changes:

  • fix 1: Fixed incompatibility between OrderDisorderedStructureTransformation and StandardTransmuter:
  • Previously, occ_tol was a method parameter in apply_transformation, making it unconfigurable when used with StandardTransmuter (which only passes transformation and optionally extend_collection parameters when calling append_transformation)
  • Moved occ_tol to class initialization to allow proper configuration when used in transformation chains

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

@Tinaatucsd Tinaatucsd force-pushed the move_occ_tol_to_init branch from 2cc623d to 107336b Compare January 28, 2025 23:46
@jmmshn
Copy link
Contributor

jmmshn commented Jan 29, 2025

Thanks @Tinaatucsd, it looks like occ_tol should have been placed as an attribute of the class since only the first two parameters are called by the alchemy part of the code.

https://github.com/materialsproject/pymatgen/blob/296a0ec856b22da847b2ccd0e55e146e071cb8fd/src/pymatgen/alchemy/materials.py#L123C1-L126C1

This was an oversight on my part. Thanks for finding this!

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.

3 participants