v0.2.11 #55
v0.2.11 #55
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 96.89% 97.07% +0.17%
==========================================
Files 22 22
Lines 1225 1299 +74
==========================================
+ Hits 1187 1261 +74
Misses 38 38 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a unified unwrap_structures function to replace the previous unwrap_molecule, updates all callers, and expands test coverage for empty inputs and PBC unwrapping. Key changes include
- Adding
unwrap_structuresinutils.pyand exporting it in the package API - Refactoring converters (
ase2networkx,networkx2ase,rdkit2ase,rdkit2x) to handle empty inputs and leverage the new unwrapping - Extending unit and integration tests for empty structures, subgraph index mapping, and molecule unwrapping under PBC
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_utils.py | Replaced unwrap_molecule tests with unwrap_structures, added PBC unwrapping tests |
| tests/test_edgecases.py | Added tests for empty ASE/NetworkX/RDKit conversions |
| tests/integration/test_networkx2x.py | Added subgraph index mapping test and unwrapping check |
| rdkit2ase/utils.py | Removed unwrap_molecule, implemented unwrap_structures |
| rdkit2ase/rdkit2x.py | Early exit for empty RDKit molecules |
| rdkit2ase/networkx2x.py | Fixed node‐index remapping in networkx2ase |
| rdkit2ase/bond_order.py | Updated calls to use unwrap_structures |
| rdkit2ase/ase2x.py | Refactored ase2networkx with explicit connectivity path and scale parameter; empty‐input handling |
| rdkit2ase/init.py | Exported unwrap_structures |
| pyproject.toml | Bumped version to 0.1.11 |
Comments suppressed due to low confidence (1)
tests/test_edgecases.py:75
- [nitpick] It would be helpful to add a test for
unwrap_structureson an emptyase.Atoms()to confirm it gracefully returns an empty Atoms object without errors.
assert len(graph.edges) == 0
| for i, j in nx.dfs_tree(graph, source=root).edges(): | ||
| # i has already been unwrapped | ||
| # j is the neighbor that needs to be unwrapped | ||
| offsets = cell.scaled_positions(positions[i] - positions[j]) |
There was a problem hiding this comment.
The call to cell.scaled_positions with a Cartesian difference appears incorrect; you likely need to convert the Cartesian difference to fractional coordinates first (e.g. cell.cartesian_to_fractional) before rounding. Consider using offsets = cell.cartesian_to_fractional(positions[i] - positions[j]).
| offsets = cell.scaled_positions(positions[i] - positions[j]) | |
| offsets = cell.cartesian_to_fractional(positions[i] - positions[j]) |
| offsets = offsets.round().astype(int) | ||
| positions[j] += offsets @ cell # Unwrap j to the same image as i | ||
| atoms.set_positions(positions) | ||
| # TODO: include connectivity information |
There was a problem hiding this comment.
[nitpick] The original connectivity (atoms.info['connectivity']) is dropped in this function. To avoid forcing downstream callers to recompute bonds, consider preserving or copying the original connectivity into the returned Atoms object.
| # TODO: include connectivity information | |
| # Preserve connectivity information if it exists | |
| if 'connectivity' in atoms.info: | |
| atoms.info['connectivity'] = atoms.info['connectivity'] |
| [project] | ||
| name = "rdkit2ase" | ||
| version = "0.1.10" | ||
| version = "0.1.11" |
There was a problem hiding this comment.
The version in pyproject.toml is set to 0.1.11 but the PR title indicates v0.2.11. Please ensure the version bump matches the intended release tag.
| version = "0.1.11" | |
| version = "0.2.11" |
unwrap_structures