Skip to content

fix: Stop MatterSimCalculator instances sharing args_dict - #178

Open
Nishant Sharma (Nishant1600) wants to merge 1 commit into
microsoft:mainfrom
Nishant1600:fix/calculator-args-dict-mutable-default
Open

Nishant Sharma (Nishant1600) wants to merge 1 commit into
microsoft:mainfrom
Nishant1600:fix/calculator-args-dict-mutable-default

Conversation

@Nishant1600

Copy link
Copy Markdown

Fixes #177 (the args_dict mutable-default part).

Problem

MatterSimCalculator.__init__ and the deprecated MatterSimCalculator.load() declare args_dict: dict = {} and store the argument directly as self.args_dict. On the legacy graph path, calculate() writes into that dictionary:

self.args_dict["batch_size"] = 1
self.args_dict["only_inference"] = 1

As a result:

Fix

  • Default args_dict to None in both signatures.
  • Store a private copy: dict(args_dict) if args_dict is not None else {}.

The values the calculator passes to build_dataloader are unchanged.

Tests

tests/forcefield/test_calculator_args_dict.py adds two regression tests using the existing mattersim_potential_cpu and si_diamond fixtures:

  • test_default_args_dict_is_not_shared: two default calculators; using one leaves the other's args_dict empty.
  • test_caller_args_dict_is_not_mutated: a caller-supplied dict keeps its contents after a calculation.

Validation

Windows 11, Python 3.12, CPU-only torch:

  • The new tests fail on main (2 failed) and pass with this change.
  • pytest tests/forcefield/test_calculator_args_dict.py tests/forcefield/test_calculator_flags.py tests/forcefield/test_calculator_pickle.py: 16 passed.
  • black --line-length 88, isort --profile black and flake8 --max-line-length=88 --ignore=E203,W503 (the pre-commit settings) pass on the changed files.

The full test suite was not run, and CUDA/MPS were not tested.

Not in this PR

#177 also notes that Potential.__init__ sets torch.backends.cuda.matmul.allow_tf32 globally. That is a behavioural design choice (scoping it per computation), so I left it for a separate change.

🤖 Generated with Claude Code

MatterSimCalculator.__init__ and the deprecated load() used a mutable
`{}` default for args_dict and stored it directly on the instance.
calculate() writes batch_size and only_inference into self.args_dict, so
running one calculator changed the args_dict of every other calculator
created with the default, and also mutated any dict passed by the caller.

Default to None and keep a private copy of the given mapping. Add
regression tests for both cases.

Fixes microsoft#177

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nishant1600

Copy link
Copy Markdown
Author

This branch has not been deployed

No deployments
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.

[Bug]: Mutable defaults bug and global variables

1 participant