Utility module updates #307
Conversation
AntObi
added
enhancement
python
WalkthroughThe changes introduce a Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #307 +/- ##
===========================================
+ Coverage 75.58% 76.55% +0.96%
===========================================
Files 24 26 +2
Lines 2183 2273 +90
===========================================
+ Hits 1650 1740 +90
Misses 533 533 ☔ View full report in Codecov by Sentry. |
![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 (2)
CONTRIBUTING.md (2)
5-21: Minor stylistic improvements.Consider the following changes for conciseness and consistency:
- Use Oxford spelling: "summarised" → "summarized".
- Remove "of" for conciseness: "all of the unit tests" → "all the unit tests".
- The steps for a new piece of work can be summarised as follows: + The steps for a new piece of work can be summarized as follows: - ensure that all of the unit tests pass on your own machine. + ensure that all the unit tests pass on your own machine.Tools
LanguageTool
[style] ~10-~10: Would you like to use the Oxford spelling “summarized”? The spelling ‘summarised’ is also correct.
Context: ...he steps for a new piece of work can be summarised as follows: 1. Push up or create [an i...(OXFORD_SPELLING_Z_NOT_S)
[style] ~17-~17: Consider removing “of” to be more concise
Context: ...are finished with the work, ensure that all of the unit tests pass on your own machine....(ALL_OF_THE)
22-31: Add a comma for clarity.Consider adding a comma for clarity: "on GitHub look" → "on GitHub, look".
- For a general overview of using pull requests on GitHub look [in the GitHub docs](https://help.github.com/en/articles/about-pull-requests). + For a general overview of using pull requests on GitHub, look [in the GitHub docs](https://help.github.com/en/articles/about-pull-requests).Tools
LanguageTool
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...eral overview of using pull requests on GitHub look [in the GitHub docs](https://help....(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~26-~26: Possible missing comma found.
Context: ...t-pull-requests). When creating a pull request you should: - Ensure that the title su...(AI_HYDRA_LEO_MISSING_COMMA)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- CONTRIBUTING.md (1 hunks)
- smact/properties.py (2 hunks)
- smact/tests/test_utils.py (1 hunks)
- smact/utils/composition.py (1 hunks)
Additional context used
LanguageTool
CONTRIBUTING.md
[style] ~10-~10: Would you like to use the Oxford spelling “summarized”? The spelling ‘summarised’ is also correct.
Context: ...he steps for a new piece of work can be summarised as follows: 1. Push up or create [an i...(OXFORD_SPELLING_Z_NOT_S)
[style] ~17-~17: Consider removing “of” to be more concise
Context: ...are finished with the work, ensure that all of the unit tests pass on your own machine....(ALL_OF_THE)
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...eral overview of using pull requests on GitHub look [in the GitHub docs](https://help....(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~26-~26: Possible missing comma found.
Context: ...t-pull-requests). When creating a pull request you should: - Ensure that the title su...(AI_HYDRA_LEO_MISSING_COMMA)
Additional comments not posted (11)
CONTRIBUTING.md (2)
1-3: LGTM!The introduction is clear and concise.
33-50: LGTM!The dev requirements section is clear and provides necessary information for setting up the development environment.
smact/tests/test_utils.py (4)
13-22: LGTM!The
setUpmethod is correctly initializing mock data for the tests.
24-49: LGTM!The
test_parse_formulamethod is comprehensive and covers various cases.
51-64: LGTM!The
test_comp_makermethod is comprehensive and covers various cases.
66-76: LGTM!The
test_formula_makermethod is comprehensive and covers various cases.smact/utils/composition.py (3)
36-51: LGTM!The
_get_sym_dictfunction is correctly implemented.
54-80: LGTM!The
comp_makerfunction is correctly implemented.
83-95: LGTM!The
formula_makerfunction is correctly implemented.smact/properties.py (2)
9-9: LGTM!The import statement for
parse_formulais correctly added to improve modularity and reusability.
Line range hint
67-105: LGTM!The
valence_electron_countfunction correctly uses the importedparse_formulato parse the compound formula.
smact/utils/composition.py
Outdated
| def parse_formula(formula: str) -> dict[str, int]: | ||
| """Parse a formula into a dict of el:amt | ||
|
|
||
| Args: | ||
| formula (str): Chemical formula | ||
|
|
||
| Returns: | ||
| dict: Dictionary of element symbol: amount | ||
| """ | ||
| regex = r"\(([^\(\)]+)\)\s*([\.e\d]*)" | ||
| r = re.compile(regex) | ||
| m = re.search(r, formula) | ||
| if m: | ||
| factor = 1.0 | ||
| if m.group(2) != "": | ||
| factor = float(m.group(2)) | ||
| unit_sym_dict = _get_sym_dict(m.group(1), factor) | ||
| expanded_sym = "".join( | ||
| [f"{el}{amt}" for el, amt in unit_sym_dict.items()] | ||
| ) | ||
| expanded_formula = formula.replace(m.group(), expanded_sym) | ||
| return parse_formula(expanded_formula) | ||
| return _get_sym_dict(formula, 1) |
There was a problem hiding this comment.
Correct the return type hint.
The return type hint should be dict[str, float] instead of dict[str, int] to accommodate non-integer stoichiometries.
-def parse_formula(formula: str) -> dict[str, int]:
+def parse_formula(formula: str) -> dict[str, float]: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.
| def parse_formula(formula: str) -> dict[str, int]: | |
| """Parse a formula into a dict of el:amt | |
| Args: | |
| formula (str): Chemical formula | |
| Returns: | |
| dict: Dictionary of element symbol: amount | |
| """ | |
| regex = r"\(([^\(\)]+)\)\s*([\.e\d]*)" | |
| r = re.compile(regex) | |
| m = re.search(r, formula) | |
| if m: | |
| factor = 1.0 | |
| if m.group(2) != "": | |
| factor = float(m.group(2)) | |
| unit_sym_dict = _get_sym_dict(m.group(1), factor) | |
| expanded_sym = "".join( | |
| [f"{el}{amt}" for el, amt in unit_sym_dict.items()] | |
| ) | |
| expanded_formula = formula.replace(m.group(), expanded_sym) | |
| return parse_formula(expanded_formula) | |
| return _get_sym_dict(formula, 1) | |
| def parse_formula(formula: str) -> dict[str, float]: | |
| """Parse a formula into a dict of el:amt | |
| Args: | |
| formula (str): Chemical formula | |
| Returns: | |
| dict: Dictionary of element symbol: amount | |
| """ | |
| regex = r"\(([^\(\)]+)\)\s*([\.e\d]*)" | |
| r = re.compile(regex) | |
| m = re.search(r, formula) | |
| if m: | |
| factor = 1.0 | |
| if m.group(2) != "": | |
| factor = float(m.group(2)) | |
| unit_sym_dict = _get_sym_dict(m.group(1), factor) | |
| expanded_sym = "".join( | |
| [f"{el}{amt}" for el, amt in unit_sym_dict.items()] | |
| ) | |
| expanded_formula = formula.replace(m.group(), expanded_sym) | |
| return parse_formula(expanded_formula) | |
| return _get_sym_dict(formula, 1) |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (3)
smact/utils/composition.py (3)
1-1: Correct the typo in the docstring.The word "functioms" should be corrected to "functions".
Apply this diff to fix the typo:
-"""Utility functioms for handling elements, species, formulas and composition""" +"""Utility functions for handling elements, species, formulas and composition"""
56-82: Improve the docstring for clarity.The docstring should specify that the function handles both two-element and three-element tuples.
Apply this diff to improve the docstring:
- """Convert an output of smact.screening.smact_filer into a Pymatgen Compositions + """Convert an output of smact.screening.smact_filter into a Pymatgen Composition.
85-97: Improve the docstring for clarity.The docstring should specify that the function uses
comp_makerto create theCompositionand then returns its reduced formula.Apply this diff to improve the docstring:
- """Convert an output of smact.screening.smact_filter into a formula. + """Convert an output of smact.screening.smact_filter into a chemical formula.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- docs/smact.utils.composition.rst (1 hunks)
- docs/smact.utils.rst (1 hunks)
- setup.py (1 hunks)
- smact/utils/init.py (1 hunks)
- smact/utils/composition.py (1 hunks)
Files skipped from review due to trivial changes (3)
- docs/smact.utils.composition.rst
- docs/smact.utils.rst
- smact/utils/init.py
Additional context used
GitHub Check: codecov/patch
smact/utils/composition.py
[warning] 50-51: smact/utils/composition.py#L50-L51
Added lines #L50 - L51 were not covered by tests
Additional comments not posted (2)
setup.py (1)
35-35: LGTM!The addition of the
smact.utilspackage to thepackageslist is necessary to include the new utilities module in the distribution setup.smact/utils/composition.py (1)
13-13: Correct the return type hint.The return type hint should be
dict[str, float]instead ofdict[str, int]to accommodate non-integer stoichiometries.Apply this diff to fix the return type hint:
-def parse_formula(formula: str) -> dict[str, int]: +def parse_formula(formula: str) -> dict[str, float]:Likely invalid or redundant comment.
| def _get_sym_dict(formula: str, factor: float) -> dict[str, float]: | ||
| sym_dict: dict[str, float] = defaultdict(float) | ||
| regex = r"([A-Z][a-z]*)\s*([-*\.e\d]*)" | ||
| r = re.compile(regex) | ||
| for m in re.finditer(r, formula): | ||
| el = m.group(1) | ||
| amt = 1.0 | ||
| if m.group(2).strip() != "": | ||
| amt = float(m.group(2)) | ||
| sym_dict[el] += amt * factor | ||
| formula = formula.replace(m.group(), "", 1) | ||
| if formula.strip(): | ||
| msg = f"{formula} is an invalid formula" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Add tests for error handling.
The function raises a ValueError if the formula is invalid. Ensure that this error handling is covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 50-51: smact/utils/composition.py#L50-L51
Added lines #L50 - L51 were not covered by tests
New utility submodule and updated Contributing guide
Description
parse_formulafromsmact.propertiesinto a separate function insmact.utils.compositionparse_formulato handle non-integer stoichiometriescomp_makerinsmact.utils.compositionwhich converts items in the list returned bysmart_filterinto a pymatgen Composition. Oxidation states are preserved if available.form_makerinsmart.utils.compositionwhich converts items in the list returned bysmart_filterinto a chemical formula.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
parse_formularefactor doesn't break exisiting testsparse_formulaworks for decimal stoichiometriescomp_makerand form_maker`Test Configuration:
Reviewers
N/A
Checklist:
Summary by CodeRabbit
New Features
CONTRIBUTING.mdfile to guide contributors on best practices and workflows.SMACT Utilities modulefor better organisation of utility functions.smact.utilssubmodule, improving accessibility.Tests
Bug Fixes