Use semantikon annotation for units in a random number of functions #467
Conversation
WalkthroughThis update introduces unit-aware typing to several scientific functions by applying the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Function
participant Semantikon
User->>Function: Call (with parameters)
Function->>Semantikon: Validate & convert units via @units decorator
Semantikon-->>Function: Return unit-checked parameters
Function-->>User: Execute calculation with unit-safe values
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #467 +/- ##
==========================================
+ Coverage 85.27% 85.34% +0.06%
==========================================
Files 43 43
Lines 2581 2593 +12
==========================================
+ Hits 2201 2213 +12
Misses 380 380 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
atomistics/workflows/phonons/helper.py (1)
280-280: Update docstring to reflect unit annotation.The docstring currently describes displacement as "float, optional" but should be updated to mention the unit.
- displacement (float, optional): The displacement distance. Defaults to 0.01. + displacement (float, optional): The displacement distance in angstroms. Defaults to 0.01.atomistics/calculators/lammps/libcalculator.py (2)
338-338: Remove debug print statement.This debug print statement should be removed before merging as it appears to be leftover from development.
- print("Force", timestep)
390-390: Add unit annotation to timestep parameter.For consistency with other functions, the timestep parameter in this function should also use the unit annotation pattern.
- timestep: float = 0.001, + timestep: u(float, units="picosecond") = 0.001,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.ci_support/environment.yml(1 hunks)atomistics/calculators/lammps/libcalculator.py(8 hunks)atomistics/workflows/phonons/helper.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
atomistics/workflows/phonons/helper.py (1)
atomistics/workflows/quasiharmonic.py (1)
generate_structures_helper(367-431)
atomistics/calculators/lammps/libcalculator.py (1)
atomistics/shared/output.py (2)
OutputMolecularDynamics(42-51)keys(11-18)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: notebooks
- GitHub Check: coverage
- GitHub Check: unittest_gpaw
🔇 Additional comments (11)
.ci_support/environment.yml (1)
19-19: Adding semantikon dependency - looks good.This addition is necessary to support the unit-aware typing and conversion features introduced in the other files.
atomistics/workflows/phonons/helper.py (2)
8-9: Adding required semantikon imports - good.These imports provide the necessary functionality for unit-aware typing and conversion.
265-269: Appropriate use of the @Units decorator.The decorator is correctly placed before the function definition, which enables the unit conversion functionality for this function.
atomistics/calculators/lammps/libcalculator.py (8)
9-10: Adding required semantikon imports - good.These imports provide the necessary functionality for unit-aware typing and conversion.
45-51: Good unit specification for force tolerance.The
ftolparameter is appropriately annotated with eV/angstrom units, which is the correct unit for force in this context.
88-94: Good unit specification for force tolerance.The
ftolparameter is consistently annotated with the same units as in the previous function.
150-159: Good unit specifications for temperature, damping, and timestep parameters.The parameters are appropriately annotated with relevant units (kelvin for temperature, picosecond for damping time and timestep).
205-217: Good unit specifications for NPT parameters.All relevant parameters (temperature, pressure, damping) are appropriately annotated with their respective units.
266-276: Good unit specifications for NPH parameters.Temperature, pressure and damping parameters are appropriately annotated with their respective units.
322-331: Good unit specifications for Langevin dynamics parameters.Temperature, damping and timestep parameters are correctly annotated with appropriate units.
380-393: Good unit specifications for thermal expansion parameters.Temperature, pressure and damping parameters are correctly annotated with appropriate units.
| def generate_structures_helper( | ||
| structure: Atoms, | ||
| primitive_matrix: Optional[np.ndarray] = None, | ||
| displacement: float = 0.01, | ||
| displacement: u(float, units="angstrom") = 0.01, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good unit specification for displacement parameter.
The displacement parameter is appropriately annotated with angstrom units, which clearly communicates its physical meaning and enables automatic unit conversion.
There is a similar function in atomistics/workflows/quasiharmonic.py that also has a displacement parameter. Consider adding unit annotations there as well for consistency:
🏁 Script executed:
#!/bin/bash
# Check if there are other similar functions that might benefit from unit annotations
grep -rn "def generate_structures_helper" --include="*.py" .Length of output: 368
🏁 Script executed:
#!/bin/bash
# Display the signature of generate_structures_helper in each file to check for displacement unit annotations
sed -n '1,40p' atomistics/workflows/elastic/helper.py
sed -n '180,240p' atomistics/workflows/evcurve/helper.py
sed -n '357,407p' atomistics/workflows/quasiharmonic.pyLength of output: 5925
Add unit annotation for displacement in QuasiHarmonic helper
The phonons helper correctly uses
displacement: u(float, units="angstrom") = 0.01to convey units, but the QuasiHarmonic generate_structures_helper still has a plain float:
• File: atomistics/workflows/quasiharmonic.py
• Around line 367 in the def generate_structures_helper(...) signature
Update the parameter to match the phonons helper:
- displacement: float = 0.01,
+ displacement: u(float, units="angstrom") = 0.01,And add the corresponding import for u if it’s not already present:
from atomistics.shared.units import u
Following @jan-janssen's request, I opened this PR for the type annotations with units using semantikon. I guess the fastest is to take a look at the actual code changes
Summary by CodeRabbit