Skip to content

Mkphuthi/intercalation calc [WIP] - #251

Open
mkphuthi wants to merge 15 commits into
materialyzeai:mainfrom
mkphuthi:mkphuthi/intercalation_calc
Open

Mkphuthi/intercalation calc [WIP]#251
mkphuthi wants to merge 15 commits into
materialyzeai:mainfrom
mkphuthi:mkphuthi/intercalation_calc

Conversation

@mkphuthi

Copy link
Copy Markdown
Contributor

Summary

Major changes:

  • feature 1: Added _mc.py, a driver for Monte-Carlo simulations (only canonical simulations implemented thus far)
  • feature 2: Added _ineracalation.py, used to predict energies as a function of concentration of an intercalating ion given a disordered or fully occupied structure thus topotactic transformations. This allows computing the free energy or just taking the lowest energy structure at each concentration
  • NOTE: This is built to support any MLIP and is actually rather slow, when using MatGL's TensorNet potentials, it is recommended to use the mc_intercalation implementation in pymacy (TBD) that takes full advantage of nvalchemi and batched simulations. I decided against implementing that in MatCalc as it goes against the ethos of benchmarking multiple potentials.

Todos

  • Document theory behind features and perform benchmarks on real system

Checklist

  • Run simulations on example systems e.g. LFP, LCO, DRX

mkphuthi and others added 11 commits June 16, 2026 16:24
…files

Adds a PropCalc subclass that removes intercalating ions from a host
structure across a range of concentrations, scoring each configuration
via single-point or relaxation and sampling with Metropolis-Hastings.
Writes a per-concentration ASE trajectory and serializes results with
monty. Exported through matcalc.__init__.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mation

Adds a PropCalc subclass that runs Metropolis-Hastings sampling using any
user-supplied transformation object (duck-typed apply_transformation). The
transform_initial flag samples configurations at a fixed concentration by
applying the transformation to the pristine structure each step, and a
trajectory plus acceptance ratio are tracked at save_freq intervals. A
grand-canonical step is stubbed for later. Exported through matcalc.__init__.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replaces the inline Metropolis-Hastings loop, boltzmann-weight helper, and
RelaxCalc construction with a per-concentration MCCalc run driven by a new
_RemoveKSites transformation (transform_initial=True for fixed-composition
sampling). IntercalationCalc keeps the concentration sweep, per-k trajectory
naming, results aggregation, and serialization. The per-level acceptance is
now reported under the key "acceptance_ratio".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds Cu/Cu_supercell/emt_calculator conftest fixtures and unit tests covering
canonical Monte Carlo sampling and the concentration sweep on a 32-atom Cu cell
with 3 vacancies, using ASE EMT for speed. One test exercises relaxation; the
rest use single-point scoring.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a swap move on a configurable sublattice for true canonical MCMC: given an
ordered host and a disordered structure marking the active sublattice, each call
relocates n_swaps occupants (species<->vacancy or species<->species), conserving
composition. Stateless, so driving it with MCCalc(transform_initial=False) gives a
genuine Markov chain. Not yet wired into IntercalationCalc.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds algorithm="markov" alongside the default "independent" sampler. Markov mode
seeds a configuration at each concentration and runs a true Metropolis chain of
local _SwapSites moves (transform_initial=False), conserving composition. Adds a
module-level _build_disordered helper (species/vacancy sublattice) and a markov
test asserting fixed composition and one swap per accepted step. The independent
path and public API are unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Avoids leaking a caller-supplied max_steps from relax_calc_kwargs into **kwargs
(which would duplicate the max_steps argument to RelaxCalc).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MCCalc now records the minimum-energy configuration visited across the chain and
exposes it as min_energy / min_structure (instance attrs and result keys). This
surfaces the ground-state ordering at each concentration in IntercalationCalc,
not just the last accepted state. Adds min_energy <= energy assertions.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Runze Liu <146490083+rul048@users.noreply.github.com>
@rul048

rul048 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution! I have resolved the conflicts and please check the output from the lint and pytest.

@shyuep

shyuep commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

🤖 Automated PR review by Claude (posted via @shyuep). Based on static analysis of the diff; no code was executed locally.

Substantial and well-documented addition — IntercalationCalc/MCCalc with clear docstrings on the move types, and the canonical vs. fixed-concentration sampling distinction is nicely explained.

Blocking: the lint job is failing (ruff check src):

  • I001 import block un-sorted in src/matcalc/__init__.py — the new _intercalation / _mc imports are out of alphabetical order. Run ruff check --fix src && ruff format src.
  • A second ruff error at src/matcalc/_intercalation.py:306.

The test suite (including fp-loaders) passes. Marked [WIP], so flagging these for when you're ready to finalize rather than as a merge gate.

shyuep and others added 4 commits August 12, 2026 07:26

@shyuep shyuep left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated PR review generated by Claude (on behalf of @shyuep)

Re-review of commits since 2026-06-30 (main merge, __init__ export, datetime/UTC cleanup, mypy signature fixes). CI green (5/5). Comments on the current state:

  1. Module-level warnings.filterwarnings("ignore", message="logm result may be inaccurate") in _intercalation.py mutates global warning state for any process that merely imports matcalc. Scope it with warnings.catch_warnings() around the relax/score call instead.
  2. MCCalc(ensemble="grand_canonical") is accepted but unimplemented — raise NotImplementedError in __init__ rather than at first use.
  3. Default trajfile="mc.traj" silently writes into the CWD; suggest None default meaning "don't write".
  4. _SwapSites (composition-conserving MCMC) vs _RemoveKSites + transform_initial=True (independent resampling, not a Markov chain): the docstrings distinguish these well, but MCCalc reports acceptance_ratio in both modes — for the resampling mode it's not a standard Metropolis acceptance ratio; a note would prevent misinterpretation.

Still [WIP] per title (benchmarks/theory docs pending), so not flagging for merge yet. Structure and test coverage look good.

@shyuep

shyuep commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Automated PR review generated by Claude (scheduled routine, posted on behalf of @shyuep) — re-review after new commits since the 2026-06-30 automated review.

The architecture has firmed up nicely: MCCalc as a generic Metropolis driver with IntercalationCalc composing it via _RemoveKSites (independent fixed-concentration sampling) and _SwapSites (composition-conserving canonical MCMC) is a clean separation, and the species↔vacancy vs binary species↔species handling in _SwapSites with tolerance-based sublattice matching is well done. CI is green.

Notes for before this leaves WIP: (1) the module-level warnings.filterwarnings("ignore", message="logm result may be inaccurate") mutates global warning state at import time — scope it with catch_warnings around the relaxation calls instead; (2) docstrings use reST :param: style while the repo standard (and the PR checklist) is Google format; (3) _RemoveKSites + transform_initial=True is independent sampling rather than a Markov chain — the docstring says so, but the class docs for IntercalationCalc should make the statistical implications explicit for users comparing against the _SwapSites canonical chain; (4) the stated todos (theory docs, LFP/LCO/DRX benchmarks) remain the main gap.

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