Conversation
Addresses missing test coverage for the symmetric strain tensor calculation in torch_sim/elastic.py:815 with tests for zero, pure shear, and hydrostatic strain cases. Verify consistency of strains produced by elementary deformations.
… convergence check (matching ASE) + test coverage - parameterized tests for `generate_force_convergence_fn` covering different tolerance levels and cell forces
WalkthroughThe changes update the build backend in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant generate_force_convergence_fn
participant SimState
User->>generate_force_convergence_fn: Call (force_tol, include_cell_forces=True)
generate_force_convergence_fn->>SimState: Access forces and cell_forces
alt include_cell_forces is True and cell_forces missing
generate_force_convergence_fn-->>User: Raise ValueError
else
generate_force_convergence_fn-->>User: Return convergence boolean tensor
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (2)tests/test_runners.py (4)
tests/test_elastic.py (4)
🪛 Pylint (3.3.7)tests/test_runners.py[refactor] 812-812: Too few public methods (0/2) (R0903) [refactor] 839-839: Too many arguments (7/5) (R0913) [refactor] 931-931: Too few public methods (0/2) (R0903) tests/test_elastic.py[refactor] 345-345: Too many local variables (17/15) (R0914) ⏰ Context from checks skipped due to timeout of 90000ms (40)
🔇 Additional comments (15)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
janosh
added
tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_runners.py (1)
804-826: Seed RNG to eliminate test flakiness
MockStatedraws random forces/cell‐forces. Rarely, those magnitudes may fall below very smallforce_tolvalues (e.g.1e-6), causing unexpected convergence and sporadic failures. Add a local seed to keep the distribution stable:def mock_state() -> Callable: @@ device = torch.device("cpu") dtype = torch.float64 + torch.manual_seed(0) # deterministic forces🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 812-812: Too few public methods (0/2)
(R0903)
tests/test_elastic.py (1)
85-88: Minor: clarify shear expectation commentThe comment mixes engineering shear strain (γ) and tensor shear ε = γ / 2.
For complete clarity to future readers, consider adding the explicit relation:-# So εyz = (u[1,2] + u[2,1])/2 = (shear_magnitude + 0)/2 = shear_magnitude/2 +# Engineering shear γyz = shear_magnitude; the symmetric strain tensor stores +# εyz = γyz / 2 = shear_magnitude / 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pyproject.toml(1 hunks)tests/test_elastic.py(4 hunks)tests/test_runners.py(2 hunks)torch_sim/runners.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_runners.py (4)
tests/conftest.py (4)
device(24-25)dtype(29-30)ar_supercell_sim_state(306-310)lj_model(34-45)torch_sim/state.py (4)
device(137-139)dtype(142-144)n_batches(161-163)SimState(26-312)torch_sim/runners.py (3)
convergence_fn(303-317)convergence_fn(334-339)generate_force_convergence_fn(287-319)tests/test_autobatching.py (1)
convergence_fn(448-456)
🪛 Pylint (3.3.7)
tests/test_elastic.py
[refactor] 343-343: Too many local variables (17/15)
(R0914)
tests/test_runners.py
[refactor] 812-812: Too few public methods (0/2)
(R0903)
[refactor] 839-839: Too many arguments (7/5)
(R0913)
[refactor] 928-928: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (38)
- GitHub Check: test-examples (examples/tutorials/metatomic_tutorial.py)
- GitHub Check: test-examples (examples/tutorials/using_graphpes_tutorial.py)
- GitHub Check: test-examples (examples/tutorials/state_tutorial.py)
- GitHub Check: test-examples (examples/tutorials/reporting_tutorial.py)
- GitHub Check: test-examples (examples/tutorials/high_level_tutorial.py)
- GitHub Check: test-examples (examples/tutorials/autobatching_tutorial.py)
- GitHub Check: test-examples (examples/scripts/5_Workflow/5.1_a2c_silicon_batched.py)
- GitHub Check: test-examples (examples/tutorials/low_level_tutorial.py)
- GitHub Check: test-examples (examples/scripts/5_Workflow/5.3_Elastic.py)
- GitHub Check: test-examples (examples/scripts/2_Structural_optimization/2.5_MACE_UnitCellFilter_Gradient_Descen...
- GitHub Check: test-examples (examples/scripts/2_Structural_optimization/2.6_MACE_UnitCellFilter_FIRE.py)
- GitHub Check: test-examples (examples/scripts/2_Structural_optimization/2.1_Lennard_Jones_FIRE.py)
- GitHub Check: test-examples (examples/scripts/6_Phonons/6.1_Phonons_MACE.py)
- GitHub Check: test-examples (examples/scripts/6_Phonons/6.3_Conductivity_MACE.py)
- GitHub Check: test-examples (examples/scripts/6_Phonons/6.2_QuasiHarmonic_MACE.py)
- GitHub Check: test-examples (examples/scripts/3_Dynamics/3.6_MACE_NVT_Nose_Hoover_temp_profile.py)
- GitHub Check: test-examples (examples/scripts/3_Dynamics/3.8_MACE_NPT_Nose_Hoover.py)
- GitHub Check: test-examples (examples/scripts/3_Dynamics/3.12_MACE_NPT_Langevin.py)
- GitHub Check: test-examples (examples/scripts/4_High_level_api/4.1_high_level_api.py)
- GitHub Check: test-examples (examples/scripts/7_Others/7.6_Compare_ASE_to_VV_FIRE.py)
- GitHub Check: test-examples (examples/scripts/7_Others/7.4_Velocity_AutoCorrelation.py)
- GitHub Check: test-model (macos-14, 3.12, lowest-direct, mace, tests/test_optimizers_vs_ase.py)
- GitHub Check: test-model (ubuntu-latest, 3.12, lowest-direct, mace, tests/test_optimizers_vs_ase.py)
- GitHub Check: test-model (macos-14, 3.11, highest, fairchem, tests/models/test_fairchem.py)
- GitHub Check: test-model (ubuntu-latest, 3.12, lowest-direct, orb, tests/models/test_orb.py)
- GitHub Check: test-model (ubuntu-latest, 3.11, highest, sevenn, tests/models/test_sevennet.py)
- GitHub Check: test-model (macos-14, 3.11, highest, mace, tests/test_elastic.py)
- GitHub Check: test-model (ubuntu-latest, 3.12, lowest-direct, sevenn, tests/models/test_sevennet.py)
- GitHub Check: test-model (ubuntu-latest, 3.12, lowest-direct, fairchem, tests/models/test_fairchem.py)
- GitHub Check: test-model (ubuntu-latest, 3.11, highest, mace, tests/test_optimizers_vs_ase.py)
- GitHub Check: test-model (ubuntu-latest, 3.11, highest, orb, tests/models/test_orb.py)
- GitHub Check: test-model (ubuntu-latest, 3.12, lowest-direct, mace, tests/test_elastic.py)
- GitHub Check: test-model (ubuntu-latest, 3.11, highest, mace, tests/test_elastic.py)
- GitHub Check: test-model (ubuntu-latest, 3.11, highest, fairchem, tests/models/test_fairchem.py)
- GitHub Check: test-core (macos-14, 3.11, highest)
- GitHub Check: test-core (ubuntu-latest, 3.12, lowest-direct)
- GitHub Check: test-core (macos-14, 3.12, lowest-direct)
- GitHub Check: build-docs
🔇 Additional comments (2)
torch_sim/runners.py (1)
288-289: Potential backward-compat break – default changed toinclude_cell_forces=TrueChanging the default alters behaviour for any caller that relied on the
previousFalsedefault and did not pass the argument explicitly.
Those calls will now raise aValueErrorunless the state already carries
cell_forces.Please double-check:
- Internal call-sites inside
torch_sim(grep forgenerate_force_convergence_fn().- External examples / docs.
If silent breakage is a concern, a deprecation cycle or a prominent changelog
entry might be warranted.pyproject.toml (1)
74-79: Build-backend switch touv_build– ensure CI & packaging images have it
uv_build(≥ 0.7.12) is now a hard requirement at build time.
Many CI images and downstream environments still ship onlyhatchling+
setuptools. Please verify:
- The project’s wheels are built in an environment with
uv0.7.12+.pip install .in a clean venv succeeds (PEP 517 path).Otherwise the package will fail to build from source for users.
Avoid undefined broadcast behavior by using torch.logical_or and torch.logical_not in tolerance ordering test
…ions_strain_consistency
1 participant
closes #208
motivation: principle of least surprise. users likely expect
torch-simrelaxations to match ASE. any changing of defaults needed to achieve this causes needless surprise.parameterized tests for
generate_force_convergence_fncovering different tolerance levels and cell forcesalso add test coverage for symmetric strain tensor calculation
(u + u.mT)/2in torch_sim/elastic.py:815 with tests for zero, pure shear, and hydrostatic strain cases.Summary by CodeRabbit