Recreate structure fresh during get_primitive_cell#144
Conversation
I mark this pull request as draft until the tests pass. |
|
I made tests pass by passing |
📝 WalkthroughWalkthroughget_primitive_cell now requires fully-periodic inputs, uses spglib’s scaled positions directly to build a new primitive Structure with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
structuretoolkit/analyse/symmetry.py (1)
388-390: Add a period at the end of the warning message.The warning message should end with a period for consistency.
- Custom arrays defined in the base structures - :attr:`ase.atoms.Atoms.arrays` are not copied to the new structure! + Custom arrays defined in the base structures + :attr:`ase.atoms.Atoms.arrays` are not copied to the new structure!.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- structuretoolkit/analyse/symmetry.py (1 hunks)
- tests/test_symmetry.py (2 hunks)
Additional context used
Ruff
structuretoolkit/analyse/symmetry.py
421-421: Undefined name
warning(F821)
Additional comments not posted (3)
tests/test_symmetry.py (2)
176-178: LGTM! The addition ofpbc=Trueis correct.The change ensures that the structure is treated as periodic, which is necessary for the
get_primitive_cellfunction to work correctly.
204-204: LGTM! The addition ofpbc=Trueis correct.The change ensures that the structure is treated as periodic, which is necessary for the
get_primitive_cellfunction to work correctly.structuretoolkit/analyse/symmetry.py (1)
392-393: Good check for periodic structures.The check ensures that the function only operates on periodic structures, which is necessary for symmetrization.
This avoids a problem, when you pass standardize=True and the resulting cell would be larger than the original. Remove array copies and add warning The trouble is that when spglib returns the symmetrized cell, it may permute, add or remove atoms so that we cannot tell anymore which values from the original arrays we would need to copy or remove.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
==========================================
+ Coverage 82.92% 82.94% +0.01%
==========================================
Files 26 26
Lines 1857 1859 +2
==========================================
+ Hits 1540 1542 +2
Misses 317 317 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the symmetry analysis workflow to rebuild a fresh Atoms object in Symmetry.get_primitive_cell(), avoiding failures when standardize=True produces a cell with more atoms than the input. It also consolidates symmetry tests and expands coverage for periodic boundary conditions and standardized FCC cells.
Changes:
- Reworked
Symmetry.get_primitive_cell()to construct a new structure from spglib’s standardized cell output (and added a strict PBC requirement). - Consolidated/expanded symmetry tests into
tests/test_analyse_symmetry.py, including new coverage forstandardize=Trueon primitive FCC. - Removed the now-redundant
tests/test_symmetry.py.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/structuretoolkit/analyse/symmetry.py |
Rebuilds a fresh structure in get_primitive_cell, adds PBC guard and warnings, and updates error handling. |
tests/test_analyse_symmetry.py |
Adds/relocates symmetry tests and updates structures to use pbc=True, including a new FCC standardization regression test. |
tests/test_symmetry.py |
Removed after moving coverage into test_analyse_symmetry.py. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .. warning:: | ||
| Custom arrays defined in the base structures | ||
| :attr:`ase.atoms.Atoms.arrays` are not copied to the new structure! | ||
| """ | ||
| if not all(self._structure.pbc): | ||
| raise ValueError("Can only symmetrize periodic structures.") | ||
| ret = spglib.standardize_cell( | ||
| self._get_spglib_cell(use_elements=use_elements, use_magmoms=use_magmoms), | ||
| to_primitive=not standardize, | ||
| ) | ||
| if ret is None: | ||
| raise SymmetryError(spglib.error.get_error_message()) | ||
| cell, positions, indices = ret | ||
| positions = (cell.T @ positions.T).T | ||
| new_structure = self._structure.copy() | ||
| new_structure.cell = cell | ||
| new_structure = new_structure[: len(indices)] | ||
| raise SymmetryError(spglib.spglib.spglib_error.message) | ||
| cell, scaled_positions, indices = ret | ||
| indices_dict = { | ||
| v: k | ||
| for k, v in structuretoolkit.common.helper.get_species_indices_dict( | ||
| structure=self._structure | ||
| ).items() | ||
| } | ||
| new_structure.symbols = [indices_dict[i] for i in indices] | ||
| new_structure.positions = positions | ||
| symbols = [indices_dict[i] for i in indices] | ||
| arrays = { | ||
| k: self._structure.arrays[k] | ||
| for k in self._structure.arrays | ||
| if k not in ("numbers", "positions") | ||
| } | ||
| new_structure = type(self._structure)( | ||
| symbols=symbols, | ||
| scaled_positions=scaled_positions, | ||
| cell=cell, | ||
| pbc=[True, True, True], | ||
| ) |
There was a problem hiding this comment.
The docstring warns only about losing custom Atoms.arrays, but the new construction type(self._structure)(...) also drops other ase.Atoms state that copy() would preserve (e.g. info, constraints, calculator, and standard per-atom arrays like initial_magmoms/tags if present). Either preserve the relevant metadata when possible or expand the warning/documentation to reflect all state that will be lost.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@samwaseda I've updated this and merge two test files that we had into one. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/structuretoolkit/analyse/symmetry.py (1)
7-7:logging.warningvswarnings.warn- different semantics.The import
from logging import warningis valid, butlogging.warning()andwarnings.warn()serve different purposes:
logging.warning()logs to the logging system (may be silently ignored if logging isn't configured)warnings.warn()issues a Python warning that users can filter/catch and is more appropriate for deprecation-style messagesFor alerting users about data loss (custom arrays not carried over),
warnings.warn()is typically more appropriate as it's more visible by default.♻️ Suggested change to use warnings.warn
-from logging import warning +import warningsAnd in line 433:
- warning(f"Custom arrays {keys} do not carry over to new structure!") + warnings.warn(f"Custom arrays {keys} do not carry over to new structure!")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/structuretoolkit/analyse/symmetry.py` at line 7, The code currently imports logging.warning and uses it to notify about custom arrays not being carried over; change the import to use the warnings module and call warnings.warn(...) instead of warning(...). Specifically, remove or replace "from logging import warning" with "import warnings" and update the call site that emits the custom-arrays/data-loss message (the warning currently emitted around the data-loss handling in symmetry.py) to use warnings.warn("...message...", category=UserWarning, stacklevel=2) so users can filter/catch the warning; keep the original message text and adjust any tests/handlers expecting logging.warning accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/structuretoolkit/analyse/symmetry.py`:
- Line 7: The code currently imports logging.warning and uses it to notify about
custom arrays not being carried over; change the import to use the warnings
module and call warnings.warn(...) instead of warning(...). Specifically, remove
or replace "from logging import warning" with "import warnings" and update the
call site that emits the custom-arrays/data-loss message (the warning currently
emitted around the data-loss handling in symmetry.py) to use
warnings.warn("...message...", category=UserWarning, stacklevel=2) so users can
filter/catch the warning; keep the original message text and adjust any
tests/handlers expecting logging.warning accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89cb8da8-d655-4a39-ad55-bf5063a6c247
📒 Files selected for processing (3)
src/structuretoolkit/analyse/symmetry.pytests/test_analyse_symmetry.pytests/test_symmetry.py
💤 Files with no reviewable changes (1)
- tests/test_symmetry.py
Co-authored-by: Jan Janssen <jan-janssen@users.noreply.github.com>
for more information, see https://pre-commit.ci
* Extend unit tests for get_primitive_cell() Added test cases to cover: - Periodic boundary condition validation: ensuring ValueError is raised if structure is not periodic. - Custom arrays warning: ensuring a warning is logged when custom arrays are present in the source structure. Co-authored-by: jan-janssen <3854739+jan-janssen@users.noreply.github.com> * fixes * another fix --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: jan-janssen <3854739+jan-janssen@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_analyse_symmetry.py (1)
332-339:⚠️ Potential issue | 🟠 MajorFix the functional
get_equivalent_pointstest to assert equivalence-class IDs, not coordinates.
test_get_equivalent_points()calls.generate_equivalent_points()and correctly asserts the minimum distance is0(the point lands on[0.51, 0.5, 0]). However,test_get_equivalent_points_functional()calls the functionalget_equivalent_points(), which delegates to.get_arg_equivalent_sites()and returns equivalence-class IDs, not coordinates. The test incorrectly treats the IDs as coordinates, computingnorm(arr - [0.51, 0.5, 0], axis=-1). Whenarr = [0](the equivalence class ID), this norm equals0.7142128534267638—exactly the norm of the coordinate vector itself—making the test pass despite checking the wrong output type.Either use
.generate_equivalent_points()to match the instance-method test, or assert the correct equivalence-class IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_analyse_symmetry.py` around lines 332 - 339, The functional test calls get_equivalent_points (which delegates to get_arg_equivalent_sites) and thus returns equivalence-class IDs, not coordinates; update test_get_equivalent_points_functional to assert the returned IDs (e.g., that arr equals the expected class id like [0] or contains 0) instead of computing distances to coordinate [0.51, 0.5, 0], or alternatively call generate_equivalent_points (matching the instance-method test) if you intended to compare coordinates; adjust the assertion to use get_equivalent_points (IDs) or generate_equivalent_points (coordinates) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_analyse_symmetry.py`:
- Around line 451-457: The test test_error currently allows either
stk.common.SymmetryError or SpglibError which masks regressions in exception
wrapping; update the assertion in tests/test_analyse_symmetry.py so the with
self.assertRaises only expects stk.common.SymmetryError (remove SpglibError),
ensuring stk.analyse.get_symmetry raises the wrapped SymmetryError as documented
and the docstring remains accurate.
---
Outside diff comments:
In `@tests/test_analyse_symmetry.py`:
- Around line 332-339: The functional test calls get_equivalent_points (which
delegates to get_arg_equivalent_sites) and thus returns equivalence-class IDs,
not coordinates; update test_get_equivalent_points_functional to assert the
returned IDs (e.g., that arr equals the expected class id like [0] or contains
0) instead of computing distances to coordinate [0.51, 0.5, 0], or alternatively
call generate_equivalent_points (matching the instance-method test) if you
intended to compare coordinates; adjust the assertion to use
get_equivalent_points (IDs) or generate_equivalent_points (coordinates)
accordingly.
🪄 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
Run ID: 7f94995a-72c3-4844-8d0c-2e1ffae75922
📒 Files selected for processing (1)
tests/test_analyse_symmetry.py
| def test_error(self): | ||
| """spglib errors should be wrapped in a SymmetryError.""" | ||
|
|
||
| structure = bulk("Al") | ||
| structure += structure[-1] | ||
| with self.assertRaises((stk.common.SymmetryError, SpglibError)): | ||
| stk.analyse.get_symmetry(structure=structure) |
There was a problem hiding this comment.
Keep this test aligned with the documented error contract.
The docstring says spglib failures are wrapped in stk.common.SymmetryError, but the assertion now also accepts raw SpglibError. That makes this test pass even if the wrapping regresses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_analyse_symmetry.py` around lines 451 - 457, The test test_error
currently allows either stk.common.SymmetryError or SpglibError which masks
regressions in exception wrapping; update the assertion in
tests/test_analyse_symmetry.py so the with self.assertRaises only expects
stk.common.SymmetryError (remove SpglibError), ensuring stk.analyse.get_symmetry
raises the wrapped SymmetryError as documented and the docstring remains
accurate.
This avoids a problem, when you pass standardize=True and the resulting cell would be larger than the original.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests