Add get_number_species_atoms helper function #383
Conversation
for more information, see https://pre-commit.ci
WalkthroughAdds a new helper function get_number_species_atoms(structure: Atoms) that counts chemical symbols from structure.get_chemical_symbols() and returns a mapping; re-exports it at package and common levels; adds unit tests verifying counts for simple structures. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Helper as get_number_species_atoms
participant Structure as Atoms
Caller->>Helper: get_number_species_atoms(structure)
Helper->>Structure: structure.get_chemical_symbols()
Structure-->>Helper: list of symbols
Helper-->>Caller: return {symbol: count}
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
==========================================
+ Coverage 83.10% 83.13% +0.02%
==========================================
Files 25 25
Lines 1859 1862 +3
==========================================
+ Hits 1545 1548 +3
Misses 314 314 ☔ 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: 2
🧹 Nitpick comments (3)
structuretoolkit/common/helper.py (3)
18-18: Prefer literal {} over dict() (ruff C408).Minor style/lint fix; consistent with the rest of the module.
- species_dict = dict() + species_dict = {}
8-16: Tighten docstring and add a return type hint.Optional polish to align with the file’s existing docstring style and add a lightweight return annotation.
-def get_number_species_atoms(structure: Atoms): - """Returns a dictionary with the species in the structure and the corresponding count in the structure +def get_number_species_atoms(structure: Atoms) -> dict: + """Return a mapping from chemical symbol to atom count. Args: - structure + structure (ase.atoms.Atoms): The atomic structure. Returns: - dict: A dictionary with the species and the corresponding count + dict: {symbol: count} Per-species atom counts. """
18-21: Optional: simplify counting using ASE’s indices.You can avoid the manual loop by leveraging
structure.symbols.indices(), which is already used elsewhere in this file.- species_dict = {} - for i in structure.get_chemical_symbols(): - species_dict[i] = species_dict.get(i, 0) + 1 - return species_dict + return {el: len(idx) for el, idx in structure.symbols.indices().items()}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
structuretoolkit/common/helper.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
structuretoolkit/common/helper.py
18-18: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
21-21: Undefined name specied_dict
(F821)
structuretoolkit/common/helper.py
Outdated
| def get_number_species_atoms(structure: Atoms): | ||
| """Returns a dictionary with the species in the structure and the corresponding count in the structure | ||
| Args: | ||
| structure | ||
| Returns: | ||
| dict: A dictionary with the species and the corresponding count | ||
| """ | ||
| species_dict = dict() | ||
| for i in structure.get_chemical_symbols(): | ||
| species_dict[i] = species_dict.get(i, 0) + 1 | ||
| return specied_dict |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add unit tests for the new helper.
A simple test with a small molecule (e.g., H2O or CO2) will prevent regressions like the current typo.
Run this to check whether tests already cover the function:
If missing, I can draft a test like:
def test_get_number_species_atoms_h2o():
from ase import Atoms
from structuretoolkit.common.helper import get_number_species_atoms
atoms = Atoms('H2O')
assert get_number_species_atoms(atoms) == {'H': 2, 'O': 1}🏁 Script executed:
#!/bin/bash
# Search for usages or tests referencing the new helper
rg -n -C2 --type=py 'get_number_species_atoms\s*\('Length of output: 392
Add missing unit tests for get_number_species_atoms
We didn’t find any existing tests covering this helper, so regressions (like the current specied_dict typo) won’t be caught automatically. Please add at least one unit test with a simple molecule—e.g., H₂O or CO₂—such as:
# tests/test_helper.py
from ase import Atoms
from structuretoolkit.common.helper import get_number_species_atoms
def test_get_number_species_atoms_h2o():
atoms = Atoms('H2O')
assert get_number_species_atoms(atoms) == {'H': 2, 'O': 1}Let me know if you’d like assistance drafting more cases.
🧰 Tools
🪛 Ruff (0.12.2)
18-18: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
21-21: Undefined name specied_dict
(F821)
🤖 Prompt for AI Agents
In structuretoolkit/common/helper.py around lines 8 to 21, the function returns
specied_dict (typo) instead of the built species_dict and there are no unit
tests to catch this; fix the function to return species_dict and add a unit test
file tests/test_helper.py that constructs a simple molecule (e.g., Atoms('H2O'))
and asserts get_number_species_atoms returns the expected mapping (e.g., {'H':
2, 'O': 1}); ensure the test imports ASE Atoms and the helper function and
include the new test in the test suite.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
structuretoolkit/common/helper.py (4)
8-17: Polish: add explicit return type and tighten the docstring.Minor typing/docs tweak for clarity and consistency with the rest of the module.
-def get_number_species_atoms(structure: Atoms): - """Returns a dictionary with the species in the structure and the corresponding count in the structure +def get_number_species_atoms(structure: Atoms) -> dict[str, int]: + """Return a mapping from chemical symbol to count. @@ - Args: - structure + Args: + structure (ase.atoms.Atoms): Atomic structure to analyze. @@ - Returns: - dict: A dictionary with the species and the corresponding count + Returns: + dict[str, int]: Counts per species.Note: If you still support Python < 3.9, prefer
-> Dict[str, int]andfrom typing import Dict.
18-21: Consider a shorter, consistent implementation using ASE indices (or numpy).This aligns with
get_species_indices_dictand avoids a manual loop. Performance-wise, it’s on par and leverages ASE’s internal symbol index mapping.- species_dict = {} - for i in structure.get_chemical_symbols(): - species_dict[i] = species_dict.get(i, 0) + 1 - return species_dict + return {el: len(indices) for el, indices in structure.symbols.indices().items()}Alternatively (uses only NumPy already imported, but sorts symbols):
symbols, counts = np.unique(structure.get_chemical_symbols(), return_counts=True) return dict(zip(symbols.tolist(), counts.tolist()))
8-8: Naming nit: considerget_species_counts(orget_number_of_atoms_per_species).This would mirror
get_species_indices_dictmore closely and read cleaner. Since it’s a new API, renaming now avoids a future breaking change.
8-21: Add a regression unit test forget_number_species_atomsI didn’t find any existing tests covering this helper—please add one to catch typos and guard against future changes. For example:
# tests/test_helper.py from ase import Atoms from structuretoolkit.common.helper import get_number_species_atoms def test_get_number_species_atoms_h2o(): atoms = Atoms('H2O') assert get_number_species_atoms(atoms) == {'H': 2, 'O': 1}• Create a new file at
tests/test_helper.py(or add to your existing test suite)
• Ensureaseis available in the test environment so this import works
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
structuretoolkit/common/helper.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unittest_old
- GitHub Check: unittest_matrix (windows-latest, 3.12, win-64-py-3-12)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.10, linux-64-py-3-10)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11, linux-64-py-3-11)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12, linux-64-py-3-12)
🔇 Additional comments (1)
structuretoolkit/common/helper.py (1)
18-21: Species-count logic is correct and straightforward.The counting over
structure.get_chemical_symbols()is functionally correct and clear.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
structuretoolkit/__init__.py(2 hunks)structuretoolkit/common/__init__.py(1 hunks)tests/test_helpers.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/test_helpers.py (1)
structuretoolkit/common/helper.py (1)
get_number_species_atoms(8-21)
structuretoolkit/__init__.py (1)
structuretoolkit/common/helper.py (1)
get_number_species_atoms(8-21)
structuretoolkit/common/__init__.py (1)
structuretoolkit/common/helper.py (1)
get_number_species_atoms(8-21)
🪛 Ruff (0.12.2)
structuretoolkit/common/__init__.py
7-7: structuretoolkit.common.helper.get_number_species_atoms imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unittest_matrix (windows-latest, 3.12, win-64-py-3-12)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.10, linux-64-py-3-10)
- GitHub Check: unittest_matrix (macos-latest, 3.12, osx-64-py-3-12)
- GitHub Check: unittest_old
🔇 Additional comments (2)
structuretoolkit/__init__.py (2)
91-96: Re-export looks good
get_number_species_atomsis correctly re-exported at the top-level namespace viafrom structuretoolkit.common import .... This enablesstructuretoolkit.get_number_species_atomsas intended.
117-117: all updated appropriatelyIncluding
"get_number_species_atoms"in__all__ensures the symbol is part of the public API surface. LGTM.
Co-authored-by: Jan Janssen <jan-janssen@users.noreply.github.com>
2 participants
Summary by CodeRabbit