Fix reading structure from output#377
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthrough
ChangesStructure reconstruction
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #377 +/- ##
=======================================
Coverage 98.77% 98.77%
=======================================
Files 13 13
Lines 1223 1223
=======================================
Hits 1208 1208
Misses 15 15 ☔ View full report in Codecov by Harness. 🚀 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 (1)
src/lammpsparser/compatibility/output.py (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
scale_atomsparameter.Add it to the docstring, including its default and its interaction with parsed positions, so generated API documentation matches the signature.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lammpsparser/compatibility/output.py` at line 9, Update the docstring for the function containing the scale_atoms parameter to document its default value of True and explain how it affects parsed positions, ensuring the generated API documentation matches the signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lammpsparser/compatibility/output.py`:
- Around line 31-36: Update the atom reconstruction flow around
atoms_copy.set_cell and atoms_copy.set_positions so scale_atoms changes the
returned coordinates as intended: assign parsed positions before set_cell when
scaling parsed coordinates is desired, or remove scale_atoms and explicitly
preserve parsed coordinates. Add tests covering both scale_atoms values and
verifying their coordinate behavior.
---
Nitpick comments:
In `@src/lammpsparser/compatibility/output.py`:
- Line 9: Update the docstring for the function containing the scale_atoms
parameter to document its default value of True and explain how it affects
parsed positions, ensuring the generated API documentation matches the
signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fe0fb77-416c-466e-8b5b-b8d6c911e10d
📒 Files selected for processing (1)
src/lammpsparser/compatibility/output.py
| atoms_copy.set_cell( | ||
| parsed_output["generic"]["cells"][index], scale_atoms=scale_atoms | ||
| ) | ||
| atoms_copy.set_array("indices", parsed_output["generic"]["indices"][index]) | ||
| atoms_copy.set_positions(parsed_output["generic"]["positions"][index]) | ||
| atoms_copy.set_velocities(parsed_output["generic"]["velocities"][index]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant file and nearby function signature/body.
if [ -f src/lammpsparser/compatibility/output.py ]; then
echo "== file outline =="
ast-grep outline src/lammpsparser/compatibility/output.py || true
echo
echo "== relevant lines =="
nl -ba src/lammpsparser/compatibility/output.py | sed -n '1,90p'
else
echo "Missing src/lammpsparser/compatibility/output.py"
echo "Candidate output.py files:"
fd -a 'output\.py$' . | sed 's#^\./##' | head -50
fi
echo
echo "== search for scale_atoms usages =="
rg -n "scale_atoms|get_scaled|set_cell\(" -S . | head -200Repository: pyiron/lammpsparser
Length of output: 317
🌐 Web query:
ASE Atoms set_cell scale_atoms semantics source
💡 Result:
In the Atomic Simulation Environment (ASE) library, the set_cell method is used to define or modify the unit cell of an Atoms object [1][2]. The scale_atoms parameter controls how the atomic positions react when the unit cell is changed [1][3]. The semantics of the scale_atoms parameter are as follows: 1. scale_atoms=False (Default): The Cartesian positions of the atoms remain unchanged in absolute space when the unit cell is modified [1][4]. This is the default behavior [1][5]. 2. scale_atoms=True: The atomic positions are scaled proportionally with the change in the unit cell [1][2]. Internally, ASE calculates a transformation matrix (M) based on the change from the old cell to the new cell and applies this transformation to the atomic positions [6][5]. This is useful for tasks such as lattice matching or system resizing where you want the fractional coordinates of the atoms to remain constant relative to the unit cell vectors [4][7]. When scale_atoms=True, the transformation is defined by: positions_new = np.dot(positions_old, M) where M is the matrix such that old_cell * M = new_cell [6][5].
Citations:
- 1: https://ase.gitlab.io/ase/ase/atoms.html
- 2: https://docs.ase-lib.org/ase/atoms.html
- 3: https://ase-lib.org/ase/atoms.html
- 4: https://docs.ase-lib.org/examples_generated/01-gettingstarted/03-manipulating-atoms.html
- 5: https://wiki.fysik.dtu.dk/~askhl/ase-doc/_modules/ase/atoms.html
- 6: https://ase-lib.org/_modules/ase/atoms.html
- 7: https://ase-lib.org/examples_generated/gettingstarted/03-manipulating-atoms.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file and function extraction =="
sed -n '1,120p' src/lammpsparser/compatibility/output.py 2>/dev/null || { python3 - <<'PY'
from pathlib import Path
p=Path('src/lammpsparser/compatibility/output.py')
print(p, p.exists(), p.stat().st_size if p.exists() else None)
PY
}
echo
echo "== deterministic order check from text =="
python3 - <<'PY'
from pathlib import Path
p=Path('src/lammpsparser/compatibility/output.py')
s=p.read_text()
terms = ['def structure_from_parsed_output', 'atoms_copy.set_cell', 'scale_atoms', 'set_array("indices"', 'set_positions(parsed_output["generic"]["positions"]']
for t in terms:
print(f"{t!r}: {s.find(t)}")
print("set_cell before set_positions:", s.index('atoms_copy.set_cell') < s.index('set_positions(parsed_output["generic"]["positions"]'))
print("scale_atoms follows set_cell args:", '"scale_atoms": scale_atoms' in s[s.index('atoms_copy.set_cell'):s.index('atoms_copy.set_cell')+500])
print("positions overwrite after set_cell:", s.index('set_positions(parsed_output["generic"]["positions"]') - s.index('atoms_copy.set_cell') < 1200)
PY
echo
echo "== related scale_atoms tests/usages =="
rg -n "scale_atoms|structure_from_parsed_output|cells\\[index\\]|positions\\[index\\]" -S . | head -200Repository: pyiron/lammpsparser
Length of output: 3380
Make scale_atoms effective, or remove the option.
set_cell() scales the template positions, but parsed positions are assigned immediately afterward, so both scale_atoms=True and scale_atoms=False return identical coordinates. Apply the parsed positions before set_cell() if scaled parsed coordinates are intended, or remove the option and preserve parsed coordinates explicitly; cover both cases in tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lammpsparser/compatibility/output.py` around lines 31 - 36, Update the
atom reconstruction flow around atoms_copy.set_cell and atoms_copy.set_positions
so scale_atoms changes the returned coordinates as intended: assign parsed
positions before set_cell when scaling parsed coordinates is desired, or remove
scale_atoms and explicitly preserve parsed coordinates. Add tests covering both
scale_atoms values and verifying their coordinate behavior.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes