Remove debug print statements from tests and replace them with assertions #210
Conversation
…ions, adjust ruff config to flag prints in package code
|
Caution Review failedThe pull request is closed. WalkthroughThis update refines linting rules and code cleanliness across the project. It upgrades the Ruff pre-commit hook, tightens enforcement of print statement warnings (except in example files), removes debug print statements from tests, replaces assertions with explicit error raises for better diagnostics, and selectively suppresses linter warnings for necessary print usage in core modules via inline comments or file-level directives. Additionally, a module graph generation script was removed. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
torch_sim/autobatching.py (1)
958-959: Consider addressing function complexity.While the
# noqa: C901comment suppresses the complexity warning, the underlying complexity issue remains. The function has substantial logic that could potentially benefit from refactoring into smaller, focused methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/_static/get_module_graph_dot_file.py(0 hunks)examples/scripts/7_Others/7.1_Soft_sphere_autograd.py(2 hunks)examples/scripts/7_Others/7.6_Compare_ASE_to_VV_FIRE.py(1 hunks)pyproject.toml(1 hunks)torch_sim/autobatching.py(2 hunks)torch_sim/models/fairchem.py(2 hunks)torch_sim/models/interface.py(3 hunks)torch_sim/models/soft_sphere.py(1 hunks)torch_sim/neighbors.py(2 hunks)torch_sim/optimizers.py(8 hunks)torch_sim/transforms.py(1 hunks)
💤 Files with no reviewable changes (1)
- docs/_static/get_module_graph_dot_file.py
✅ Files skipped from review due to trivial changes (1)
- examples/scripts/7_Others/7.1_Soft_sphere_autograd.py
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- torch_sim/models/fairchem.py
- torch_sim/optimizers.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
torch_sim/autobatching.py (1)
torch_sim/state.py (2)
SimState(26-312)n_batches(161-163)
🪛 GitHub Actions: Build docs
torch_sim/neighbors.py
[error] 113-113: TorchScript compilation failed with NotSupportedError: Don't support conversion in JoinedStr at line 113, related to f-string conversion in raise statement.
🪛 Pylint (3.3.7)
torch_sim/models/interface.py
[refactor] 202-202: Too many local variables (21/15)
(R0914)
[refactor] 202-202: Too many branches (20/12)
(R0912)
[refactor] 202-202: Too many statements (57/50)
(R0915)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-examples (examples/scripts/6_Phonons/6.1_Phonons_MACE.py)
- GitHub Check: test-examples (examples/scripts/3_Dynamics/3.2_MACE_NVE.py)
- GitHub Check: test-examples (examples/scripts/3_Dynamics/3.13_MACE_NVE_non_pbc.py)
- GitHub Check: test-model (macos-14, 3.12, lowest-direct, mattersim, tests/models/test_mattersim.py)
- GitHub Check: test-model (macos-14, 3.12, lowest-direct, orb, tests/models/test_orb.py)
- GitHub Check: test-model (macos-14, 3.12, lowest-direct, sevenn, tests/models/test_sevennet.py)
- GitHub Check: test-model (macos-14, 3.11, highest, metatomic, tests/models/test_metatomic.py)
🔇 Additional comments (9)
examples/scripts/7_Others/7.6_Compare_ASE_to_VV_FIRE.py (1)
487-488: Excellent improvement to error handling.Replacing the assertion with explicit conditional checking and
ValueErroris a significant improvement. This change ensures the validation cannot be disabled with Python's-Oflag and provides a more descriptive error message that includes the actual variable value, making debugging easier for users of this example script.torch_sim/transforms.py (1)
524-527: Strong improvement to input validation.The replacement of assertions with explicit conditional checks that raise
ValueErroris an excellent change for library code. The new error messages include the actual shape information (got {mapping.shape}), which will significantly help users debug invalid inputs. This approach ensures validation cannot be bypassed and follows Python conventions for argument validation.torch_sim/models/soft_sphere.py (1)
645-648: Excellent consolidation and improvement of validation logic.This change represents a significant improvement in both maintainability and error handling:
- Better maintainability: Consolidating the symmetry checks into a loop makes it easier to add new parameter matrices in the future
- Improved error reporting: The error message now specifies which matrix failed the symmetry check
- Robust validation: Replaces assertions with explicit
ValueErrorraising that cannot be disabledThe use of
getattr()with the matrix name provides both flexibility and clear error messages, making this a model example of how to handle similar validation patterns.torch_sim/models/interface.py (5)
234-236: Excellent improvement to error handling.The replacement of assertions with explicit conditional checks and informative ValueError exceptions significantly improves debugging and error reporting. This ensures validation checks remain active even with Python optimization flags.
265-272: LGTM - Robust input validation.The explicit checks for input tensor mutation with detailed error messages provide much better diagnostics than the previous assertions.
275-280: Well-structured validation logic.The systematic checking of required output keys with explicit exceptions improves code reliability and maintainability.
283-288: Clear and informative shape validation.The explicit shape validation with descriptive error messages will greatly assist in debugging model implementation issues.
294-317: Comprehensive output consistency validation.The detailed validation of single-batch vs multi-batch consistency with explicit error messages and tolerance values provides excellent debugging information for model implementations.
torch_sim/autobatching.py (1)
1023-1030: Excellent validation improvements.The systematic replacement of assertions with explicit ValueError exceptions provides much better error reporting and ensures these critical validations remain active in optimized Python environments. The error messages clearly indicate the expected vs actual values.
janosh
force-pushed
the
ruff-disallow-prints
branch
from
adf9f59 to
78c3690
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/models/test_soft_sphere.py (1)
330-332: Make thematchregex less brittle
pytest.raises(ValueError, match="is not symmetric")will fail if the error message is re-phrased (e.g. “matrix … is not symmetric”).
A slightly more resilient pattern keeps the essence while tolerating wording tweaks:-with pytest.raises(ValueError, match="is not symmetric"): +with pytest.raises(ValueError, match=r"is\s+not\s+symmetric"):The
\s+wildcard shields the test from minor spacing/punctuation edits without weakening the assertion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/models/test_soft_sphere.py(1 hunks)torch_sim/neighbors.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
torch_sim/neighbors.py
[warning] 112-113: torch_sim/neighbors.py#L112-L113
Added lines #L112 - L113 were not covered by tests
[warning] 218-221: torch_sim/neighbors.py#L218-L221
Added lines #L218 - L221 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (38)
- GitHub Check: test-examples (examples/tutorials/reporting_tutorial.py)
- GitHub Check: test-examples (examples/tutorials/low_level_tutorial.py)
- GitHub Check: test-examples (examples/scripts/2_Structural_optimization/2.7_MACE_FrechetCellFilter_FIRE.py)
- GitHub Check: test-examples (examples/tutorials/high_level_tutorial.py)
- GitHub Check: test-examples (examples/tutorials/hybrid_swap_tutorial.py)
- GitHub Check: test-examples (examples/scripts/2_Structural_optimization/2.4_MACE_FIRE.py)
- GitHub Check: test-examples (examples/tutorials/autobatching_tutorial.py)
- GitHub Check: test-examples (examples/scripts/2_Structural_optimization/2.3_MACE_Gradient_Descent.py)
- GitHub Check: test-examples (examples/scripts/5_Workflow/5.3_Elastic.py)
- GitHub Check: test-examples (examples/scripts/5_Workflow/5.1_a2c_silicon_batched.py)
- GitHub Check: test-examples (examples/scripts/3_Dynamics/3.6_MACE_NVT_Nose_Hoover_temp_profile.py)
- GitHub Check: test-examples (examples/scripts/6_Phonons/6.2_QuasiHarmonic_MACE.py)
- GitHub Check: test-examples (examples/scripts/3_Dynamics/3.10_Hybrid_swap_mc.py)
- GitHub Check: test-examples (examples/scripts/4_High_level_api/4.1_high_level_api.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/7_Others/7.4_Velocity_AutoCorrelation.py)
- GitHub Check: test-examples (examples/scripts/7_Others/7.6_Compare_ASE_to_VV_FIRE.py)
- GitHub Check: test-model (macos-14, 3.12, lowest-direct, 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, orb, tests/models/test_orb.py)
- GitHub Check: test-model (macos-14, 3.11, highest, mace, tests/test_elastic.py)
- GitHub Check: test-model (ubuntu-latest, 3.11, highest, sevenn, tests/models/test_sevennet.py)
- GitHub Check: test-model (ubuntu-latest, 3.11, highest, mace, tests/test_elastic.py)
- GitHub Check: test-model (ubuntu-latest, 3.12, lowest-direct, mace, tests/test_elastic.py)
- GitHub Check: test-model (macos-14, 3.11, highest, graphpes, tests/models/test_graphpes.py)
- GitHub Check: test-model (ubuntu-latest, 3.11, highest, orb, tests/models/test_orb.py)
- GitHub Check: test-model (ubuntu-latest, 3.11, highest, mace, tests/test_optimizers_vs_ase.py)
- GitHub Check: test-model (ubuntu-latest, 3.12, lowest-direct, graphpes, tests/models/test_graphpes.py)
- GitHub Check: test-model (ubuntu-latest, 3.12, lowest-direct, mace, tests/test_optimizers_vs_ase.py)
- GitHub Check: test-model (ubuntu-latest, 3.11, highest, graphpes, tests/models/test_graphpes.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, 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 (ubuntu-latest, 3.11, highest)
- GitHub Check: test-core (macos-14, 3.12, lowest-direct)
- GitHub Check: build-docs
🔇 Additional comments (1)
torch_sim/neighbors.py (1)
112-113:raise ValueErrorinsidetorch.jit.scriptcan break JIT compilationTorchScript currently guarantees support only for
RuntimeError.
Trying to compile this function will fail withTorchScriptException: Exception cannot be raised from TorchScript code:in older & some current PyTorch versions.
Safer:
-if face_dist_c.shape != (3,): - raise ValueError(f"face_dist_c.shape={face_dist_c.shape} != (3,)") +if face_dist_c.shape != (3,): + raise RuntimeError(f"face_dist_c.shape={face_dist_c.shape} != (3,)")Same remark applies to the similar checks below.
Please runtorch.jit.script(primitive_neighbor_list)locally to confirm.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 112-113: torch_sim/neighbors.py#L112-L113
Added lines #L112 - L113 were not covered by tests
adjust ruff config to flag prints in package code
Summary by CodeRabbit
Style
Chores