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 in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.githubusercontent.com/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.
| 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], | ||
| ) | ||
| keys = set(arrays) - {"numbers", "positions"} | ||
| if len(keys) > 0: | ||
| warning(f"Custom arrays {keys} do not carry over to new structure!") |
There was a problem hiding this comment.
Fix the undefined name warning.
The warning function is not defined. Use the warnings.warn function instead.
- if len(keys) > 0:
- warning(f"Custom arrays {keys} do not carry over to new structure!")
+ if len(keys) > 0:
+ import warnings
+ warnings.warn(f"Custom arrays {keys} do not carry over to new structure!")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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], | |
| ) | |
| keys = set(arrays) - {"numbers", "positions"} | |
| if len(keys) > 0: | |
| warning(f"Custom arrays {keys} do not carry over to new structure!") | |
| 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], | |
| ) | |
| keys = set(arrays) - {"numbers", "positions"} | |
| if len(keys) > 0: | |
| import warnings | |
| warnings.warn(f"Custom arrays {keys} do not carry over to new structure!") |
Tools
Ruff
421-421: Undefined name
warning(F821)
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
==========================================
- Coverage 82.69% 82.60% -0.10%
==========================================
Files 25 25
Lines 1832 1834 +2
==========================================
Hits 1515 1515
- Misses 317 319 +2 ☔ 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.
| import string | ||
| from logging import warning | ||
|
|
There was a problem hiding this comment.
from logging import warning + calling warning(...) logs on the root logger and is inconsistent with the rest of the codebase, which uses warnings.warn(..., stacklevel=2) for user-facing notices (e.g. analyze/neighbors.py). Consider switching to warnings.warn (or a module-level logger via logging.getLogger(__name__)) so callers can filter/handle this message predictably.
| .. 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
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