Merge develop into master branch #306
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Magpie data updates
Nvalence_modification
Dependabot updates
properties.py
invalid elements ie Lr0
VEC function integration into smact.properties with a test file to serve as an example for its use.
WalkthroughThe changes encompass updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script
participant Properties
participant DataLoader
User->>Script: Request VEC for "Fe2O3"
Script->>Properties: Call valence_electron_count("Fe2O3")
Properties->>DataLoader: Retrieve valence data for "Fe2O3"
DataLoader-->>Properties: Return valence data
Properties-->>Script: Return calculated VEC
Script-->>User: Display VEC
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 @@
## master #306 +/- ##
==========================================
+ Coverage 75.47% 76.46% +0.99%
==========================================
Files 24 26 +2
Lines 2361 2265 -96
==========================================
- Hits 1782 1732 -50
+ Misses 579 533 -46 ☔ 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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
smact/data/element_valence_modified.csvis excluded by!**/*.csvsmact/data/magpie.csvis excluded by!**/*.csv
Files selected for processing (7)
- .pre-commit-config.yaml (2 hunks)
- examples/Crystal_Space/1_reduction.ipynb (1 hunks)
- examples/vec_example.py (1 hunks)
- smact/init.py (3 hunks)
- smact/data_loader.py (2 hunks)
- smact/properties.py (2 hunks)
- smact/tests/test_core.py (2 hunks)
Files skipped from review due to trivial changes (2)
- examples/Crystal_Space/1_reduction.ipynb
- examples/vec_example.py
Additional context used
Ruff
smact/properties.py
1-1:
csvimported but unusedRemove unused import:
csv(F401)
211-211: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
smact/__init__.py
168-171: Use ternary operator
num_valence_modified = valence_data["NValence"] if valence_data else Noneinstead ofif-else-blockReplace
if-else-block withnum_valence_modified = valence_data["NValence"] if valence_data else None(SIM108)
Additional comments not posted (15)
.pre-commit-config.yaml (2)
7-7: LGTM!The formatting correction improves readability and ensures the arguments are correctly separated.
The code changes are approved.
19-23: LGTM!Updating the
nbQAversion and altering the arguments for thenbqa-pyupgradehook ensures compatibility with newer Python standards and potentially includes additional features or bug fixes.The code changes are approved.
smact/properties.py (1)
196-199: Useraise ... from Nonefor raising exceptions.To distinguish exceptions raised in the
get_element_valencefunction from errors in exception handling, useraise ... from None.Apply this diff to improve exception handling:
- except NameError: + except NameError as e: raise ValueError( - f"Valence data not found for element: {element}" - ) from None + f"Valence data not found for element: {element}" + ) from eLikely invalid or redundant comment.
smact/tests/test_core.py (1)
94-110: LGTM!The test cases are comprehensive and ensure the robustness of the
valence_electron_countfunction.The code changes are approved.
smact/__init__.py (7)
76-76: LGTM!The attribute
mendeleevis correctly added and documented.The code changes are approved.
78-78: LGTM!The attribute
AtomicWeightis correctly added and documented.The code changes are approved.
80-80: LGTM!The attribute
MeltingTis correctly added and documented.The code changes are approved.
82-82: LGTM!The attribute
num_valenceis correctly added and documented.The code changes are approved.
84-84: LGTM!The attribute
num_valence_modifiedis correctly added and documented.The code changes are approved.
155-161: LGTM!The data retrieval and initialization for
magpie_dataare correctly implemented.The code changes are approved.
207-211: LGTM!The attribute setting is correctly implemented.
The code changes are approved.
smact/data_loader.py (4)
829-895: LGTM!The data initialization and caching for
_element_magpie_dataare correctly implemented.The code changes are approved.
897-906: LGTM!The data retrieval and warning mechanism are correctly implemented.
The code changes are approved.
909-947: LGTM!The data initialization and caching for
_element_valence_dataare correctly implemented.The code changes are approved.
948-957: LGTM!The data retrieval and warning mechanism are correctly implemented.
The code changes are approved.
smact/properties.py
Outdated
| @@ -1,3 +1,6 @@ | |||
| import csv | |||
There was a problem hiding this comment.
Remove unused import.
The csv module is imported but not used in the file.
Apply this diff to remove the unused import:
-import csvCommittable 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.
| import csv |
Tools
Ruff
1-1:
csvimported but unusedRemove unused import:
csv(F401)
smact/__init__.py
Outdated
| valence_data = data_loader.lookup_element_valence_data(symbol) | ||
| if valence_data: | ||
| num_valence_modified = valence_data["NValence"] | ||
| else: | ||
| num_valence_modified = None |
There was a problem hiding this comment.
Consider using a ternary operator for conciseness.
Replace the if-else block with a ternary operator for the num_valence_modified attribute.
Apply this diff to use a ternary operator:
- valence_data = data_loader.lookup_element_valence_data(symbol)
- if valence_data:
- num_valence_modified = valence_data["NValence"]
- else:
- num_valence_modified = None
+ num_valence_modified = valence_data["NValence"] if valence_data else NoneCommittable 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.
| valence_data = data_loader.lookup_element_valence_data(symbol) | |
| if valence_data: | |
| num_valence_modified = valence_data["NValence"] | |
| else: | |
| num_valence_modified = None | |
| valence_data = data_loader.lookup_element_valence_data(symbol) | |
| num_valence_modified = valence_data["NValence"] if valence_data else None |
Tools
Ruff
168-171: Use ternary operator
num_valence_modified = valence_data["NValence"] if valence_data else Noneinstead ofif-else-blockReplace
if-else-block withnum_valence_modified = valence_data["NValence"] if valence_data else None(SIM108)
Utility module updates
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (4)
CONTRIBUTING.md (2)
5-21: Improve readability.Consider using Oxford spelling and removing "of" for conciseness.
- 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 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: Improve readability.Consider adding a comma for clarity.
- 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)
smact/properties.py (2)
Line range hint
9-28: Improve documentation and use a more specific exception.
- Improve the docstring to provide more details about the function's behaviour.
- Use a more specific exception instead of a generic
Exception.Improve the docstring.
def eneg_mulliken(element: Union[smact.Element, str]) -> float: """Get Mulliken electronegativity from the IE and EA. Arguments: symbol (smact.Element or str): Element object or symbol Returns: mulliken (float): Mulliken electronegativity """Use a more specific exception.
def eneg_mulliken(element: Union[smact.Element, str]) -> float: if type(element) == str: element = smact.Element(element) elif type(element) != smact.Element: - raise Exception(f"Unexpected type: {type(element)}") + raise TypeError(f"Unexpected type: {type(element)}") mulliken = (element.ionpot + element.e_affinity) / 2.0 return mullikenTools
Ruff
1-1:
csvimported but unusedRemove unused import:
csv(F401)
2-2:
reimported but unusedRemove unused import:
re(F401)
3-3:
collections.defaultdictimported but unusedRemove unused import:
collections.defaultdict(F401)
Line range hint
29-73: Improve documentation and use type hints.
- Improve the docstring to provide more details about the function's behaviour.
- Use type hints for the arguments and return value.
Improve the docstring.
def band_gap_Harrison( anion: str, cation: str, verbose: bool = False, distance: Optional[Union[float, str]] = None, ) -> float: """ Estimates the band gap from elemental data. The band gap is estimated using the principles outlined in Harrison's 1980 work "Electronic Structure and the Properties of Solids: The Physics of the Chemical Bond". Args: Anion (str): Element symbol of the dominant anion in the system Cation (str): Element symbol of the the dominant cation in the system Distance (float or str): Nuclear separation between anion and cation i.e. sum of ionic radii verbose (bool) : An optional True/False flag. If True, additional information is printed to the standard output. [Defult: False] Returns : Band_gap (float): Band gap in eV """Use type hints for the arguments and return value.
def band_gap_Harrison( anion: str, cation: str, verbose: bool = False, distance: Optional[Union[float, str]] = None, ) -> float:Tools
Ruff
1-1:
csvimported but unusedRemove unused import:
csv(F401)
2-2:
reimported but unusedRemove unused import:
re(F401)
3-3:
collections.defaultdictimported but unusedRemove unused import:
collections.defaultdict(F401)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- CONTRIBUTING.md (1 hunks)
- docs/smact.rst (1 hunks)
- docs/smact.utils.composition.rst (1 hunks)
- docs/smact.utils.rst (1 hunks)
- setup.py (1 hunks)
- smact/properties.py (2 hunks)
- smact/tests/test_utils.py (1 hunks)
- smact/utils/init.py (1 hunks)
- smact/utils/composition.py (1 hunks)
Files skipped from review due to trivial changes (4)
- docs/smact.rst
- docs/smact.utils.composition.rst
- docs/smact.utils.rst
- smact/utils/init.py
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)
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
Ruff
smact/properties.py
1-1:
csvimported but unusedRemove unused import:
csv(F401)
2-2:
reimported but unusedRemove unused import:
re(F401)
3-3:
collections.defaultdictimported but unusedRemove unused import:
collections.defaultdict(F401)
202-202: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Additional comments not posted (6)
CONTRIBUTING.md (2)
1-3: LGTM!The introduction is clear and concise.
33-50: LGTM!The dev requirements section is clear and provides detailed instructions.
setup.py (1)
35-35: LGTM!The modification ensures that the
smact.utilsmodule is included in the package distribution.smact/tests/test_utils.py (3)
24-49: LGTM!The test function is comprehensive and covers various cases.
51-64: LGTM!The test function is comprehensive and covers various cases.
66-76: LGTM!The test function is comprehensive and covers various cases.
smact/utils/composition.py
Outdated
| 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.
Improve efficiency and documentation.
- Compile the regular expressions once and reuse them.
- Improve the docstring to provide more details about the function's behaviour.
Compile the regular expressions once and reuse them.
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)
+ r = re.compile(r"\(([^\(\)]+)\)\s*([\.e\d]*)")
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)Improve the docstring.
def parse_formula(formula: str) -> dict[str, float]:
- """Parse a formula into a dict of el:amt
+ """Parse a chemical formula into a dictionary of elements and their amounts.
Args:
formula (str): Chemical formula
Returns:
dict: Dictionary of element symbol: amount
"""| 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) | ||
|
|
||
| return sym_dict |
There was a problem hiding this comment.
Improve efficiency and documentation.
- Compile the regular expressions once and reuse them.
- Improve the docstring to provide more details about the function's behaviour.
Compile the regular expressions once and reuse them.
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)
+ r = re.compile(r"([A-Z][a-z]*)\s*([-*\.e\d]*)")
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)
return sym_dictImprove the docstring.
def _get_sym_dict(formula: str, factor: float) -> dict[str, float]:
+ """Convert a chemical formula into a dictionary of elements and their amounts.
+ Args:
+ formula (str): Chemical formula
+ factor (float): Multiplication factor for the amounts
+ Returns:
+ dict: Dictionary of element symbol: amount
+ Raises:
+ ValueError: If the formula is invalid
+ """
sym_dict: dict[str, float] = defaultdict(float)Tools
GitHub Check: codecov/patch
[warning] 50-51: smact/utils/composition.py#L50-L51
Added lines #L50 - L51 were not covered by tests
smact/utils/composition.py
Outdated
| def comp_maker( | ||
| smact_filter_output: tuple[str, int, int] | tuple[str, int] | ||
| ) -> Composition: | ||
| """Convert an output of smact.screening.smact_filer into a Pymatgen Compositions | ||
|
|
||
| Args: | ||
| smact_filter_output (tuple[str, int, int]|tuple[str, int]): An item in the list returned from smact_filter | ||
|
|
||
| Returns: | ||
| composition (pymatgen.core.Composition): An instance of the Composition class | ||
| """ | ||
| if len(smact_filter_output) == 2: | ||
| form = [] | ||
| for el, ammt in zip(smact_filter_output[0], smact_filter_output[-1]): | ||
| form.append(el) | ||
| form.append(ammt) | ||
| form = "".join(str(e) for e in form) | ||
| else: | ||
| form = {} | ||
| for el, ox, ammt in zip( | ||
| smact_filter_output[0], | ||
| smact_filter_output[1], | ||
| smact_filter_output[2], | ||
| ): | ||
| sp = unparse_spec((el, ox)) | ||
| form[sp] = ammt | ||
| return Composition(form) |
There was a problem hiding this comment.
Improve documentation and simplify the function.
- Improve the docstring to provide more details about the function's behaviour.
- Simplify the function using a dictionary comprehension.
Improve the docstring.
def comp_maker(
smact_filter_output: tuple[str, int, int] | tuple[str, int]
) -> Composition:
- """Convert an output of smact.screening.smact_filer into a Pymatgen Compositions
+ """Convert an output of smact.screening.smact_filter into a Pymatgen Composition.
Args:
smact_filter_output (tuple[str, int, int]|tuple[str, int]): An item in the list returned from smact_filter
Returns:
composition (pymatgen.core.Composition): An instance of the Composition class
"""Simplify the function using a dictionary comprehension.
def comp_maker(
smact_filter_output: tuple[str, int, int] | tuple[str, int]
) -> Composition:
if len(smact_filter_output) == 2:
form = []
for el, ammt in zip(smact_filter_output[0], smact_filter_output[-1]):
form.append(el)
form.append(ammt)
form = "".join(str(e) for e in form)
else:
- form = {}
- for el, ox, ammt in zip(
- smact_filter_output[0],
- smact_filter_output[1],
- smact_filter_output[2],
- ):
- sp = unparse_spec((el, ox))
- form[sp] = ammt
+ form = {
+ unparse_spec((el, ox)): ammt
+ for el, ox, ammt in zip(
+ smact_filter_output[0],
+ smact_filter_output[1],
+ smact_filter_output[2],
+ )
+ }
return Composition(form)
smact/utils/composition.py
Outdated
| def formula_maker( | ||
| smact_filter_output: tuple[str, int, int] | tuple[str, int] | ||
| ) -> str: | ||
| """Convert an output of smact.screening.smact_filter into a formula. | ||
|
|
||
| Args: | ||
| smact_filter_output (tuple[str, int, int]|tuple[str, int]): An item in the list returned from smact_filter | ||
|
|
||
| Returns: | ||
| formula (str): A formula | ||
|
|
||
| """ | ||
| return comp_maker(smact_filter_output).reduced_formula |
There was a problem hiding this comment.
Improve documentation.
Improve the docstring to provide more details about the function's behaviour.
Improve the docstring.
def formula_maker(
smact_filter_output: tuple[str, int, int] | tuple[str, int]
) -> str:
- """Convert an output of smact.screening.smact_filter into a formula.
+ """Convert an output of smact.screening.smact_filter into a chemical formula.
Args:
smact_filter_output (tuple[str, int, int]|tuple[str, int]): An item in the list returned from smact_filter
Returns:
formula (str): A chemical formula
"""Committable suggestion was skipped due to low confidence.
smact/properties.py
Outdated
| def valence_electron_count(compound: str) -> float: | ||
| """ | ||
| Calculate the Valence Electron Count (VEC) for a given compound. | ||
|
|
||
| This function parses the input compound, extracts the elements and their | ||
| stoichiometries, and calculates the VEC using the valence electron data | ||
| from SMACT's Element class. | ||
|
|
||
| Args: | ||
| compound (str): Chemical formula of the compound (e.g., "Fe2O3"). | ||
|
|
||
| Returns: | ||
| float: Valence Electron Count (VEC) for the compound. | ||
|
|
||
| Raises: | ||
| ValueError: If an element in the compound is not found in the valence data. | ||
| """ | ||
|
|
||
| def get_element_valence(element: str) -> int: | ||
| try: | ||
| return smact.Element(element).num_valence_modified | ||
| except NameError: | ||
| raise ValueError( | ||
| f"Valence data not found for element: {element}" | ||
| ) from None | ||
|
|
||
| element_stoich = parse_formula(compound) | ||
|
|
||
| total_valence = 0 | ||
| total_stoich = 0 | ||
| for element, stoich in element_stoich.items(): | ||
| try: | ||
| valence = get_element_valence(element) | ||
| total_valence += stoich * valence | ||
| total_stoich += stoich | ||
| except TypeError: | ||
| raise ValueError(f"No valence information for element {element}") | ||
|
|
||
| if total_stoich == 0: | ||
| return 0.0 | ||
|
|
||
| vec = total_valence / total_stoich | ||
| return vec |
There was a problem hiding this comment.
Improve documentation, readability, and error handling.
- Improve the docstring to provide more details about the function's behaviour.
- Move the nested function
get_element_valenceoutside to improve readability. - Improve the error handling by using
raise ... from None.
Improve the docstring.
def valence_electron_count(compound: str) -> float:
"""
- Calculate the Valence Electron Count (VEC) for a given compound.
+ Calculate the Valence Electron Count (VEC) for a given chemical compound.
This function parses the input compound, extracts the elements and their
stoichiometries, and calculates the VEC using the valence electron data
from SMACT's Element class.
Args:
compound (str): Chemical formula of the compound (e.g., "Fe2O3").
Returns:
float: Valence Electron Count (VEC) for the compound.
Raises:
ValueError: If an element in the compound is not found in the valence data.
"""Move the nested function get_element_valence outside.
+def get_element_valence(element: str) -> int:
+ try:
+ return smact.Element(element).num_valence_modified
+ except NameError:
+ raise ValueError(
+ f"Valence data not found for element: {element}"
+ ) from None
+
def valence_electron_count(compound: str) -> float:
- def get_element_valence(element: str) -> int:
- try:
- return smact.Element(element).num_valence_modified
- except NameError:
- raise ValueError(
- f"Valence data not found for element: {element}"
- ) from None
element_stoich = parse_formula(compound)
total_valence = 0
total_stoich = 0
for element, stoich in element_stoich.items():
try:
valence = get_element_valence(element)
total_valence += stoich * valence
total_stoich += stoich
except TypeError:
- raise ValueError(f"No valence information for element {element}")
+ raise ValueError(f"No valence information for element {element}") from None
if total_stoich == 0:
return 0.0
vec = total_valence / total_stoich
return vecTools
Ruff
202-202: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
smact/properties.py
Outdated
| import csv | ||
| import re | ||
| from collections import defaultdict |
There was a problem hiding this comment.
Remove unused imports.
The csv, re, and collections.defaultdict modules are imported but not used in the file.
Apply this diff to remove the unused imports:
-import csv
-import re
-from collections import defaultdictCommittable 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.
| import csv | |
| import re | |
| from collections import defaultdict |
Tools
Ruff
1-1:
csvimported but unusedRemove unused import:
csv(F401)
2-2:
reimported but unusedRemove unused import:
re(F401)
3-3:
collections.defaultdictimported but unusedRemove unused import:
collections.defaultdict(F401)
Develop branch updates in preparation for new release
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- docs/conf.py (1 hunks)
- setup.py (2 hunks)
- smact/init.py (5 hunks)
- smact/properties.py (2 hunks)
- smact/screening.py (6 hunks)
- smact/utils/composition.py (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/conf.py
Files skipped from review as they are similar to previous changes (2)
- setup.py
- smact/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
Ruff
smact/properties.py
199-199: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Additional comments not posted (7)
smact/utils/composition.py (4)
12-35: Approve changes but consider previous suggestions.The function correctly parses chemical formulas. However, consider applying the previously suggested improvements to compile the regular expressions once and enhance the docstring for better clarity.
The code changes are approved.
37-53: Approve changes but consider previous suggestions.The function correctly converts chemical formulas into dictionaries. However, consider applying the previously suggested improvements to compile the regular expressions once and enhance the docstring for better clarity.
The code changes are approved.
Tools
GitHub Check: codecov/patch
[warning] 50-51: smact/utils/composition.py#L50-L51
Added lines #L50 - L51 were not covered by tests
56-82: Approve changes but consider previous suggestions.The function correctly converts outputs into
Pymatgencompositions. However, consider applying the previously suggested improvements to enhance the docstring and simplify the function using a dictionary comprehension.The code changes are approved.
85-97: Approve changes but consider previous suggestions.The function correctly converts outputs into chemical formulas. However, consider applying the previously suggested improvements to enhance the docstring for better clarity.
The code changes are approved.
smact/properties.py (1)
163-205: Approve changes but consider previous suggestions.The function correctly calculates the Valence Electron Count (VEC) for chemical compounds. However, consider applying the previously suggested improvements to enhance the docstring, improve error handling, and move the nested function outside for better readability.
The code changes are approved.
Tools
Ruff
199-199: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
smact/screening.py (2)
Line range hint
324-392: Approve changes but ensure clear documentation.The function correctly applies tests for chemical compositions. Ensure that the documentation clearly explains the new default oxidation states and provides guidance for users transitioning from earlier versions.
The code changes are approved.
Line range hint
437-502: Approve changes but ensure clear documentation.The function correctly checks the validity of compositions. Ensure that the documentation clearly explains the new default oxidation states and provides guidance for users transitioning from earlier versions.
The code changes are approved.
AntObi
added
bug
docs
dependencies
AntObi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- README.md (1 hunks)
- requirements.txt (5 hunks)
Additional context used
LanguageTool
README.md
[grammar] ~125-~125: The pronoun ‘they’ must be used with a non-third-person form of a verb.
Context: ...n the issue tracker, especially if they helps us improve the docs! For other queries ...(NON3PRS_VERB)
[style] ~128-~128: Consider a different adverb to strengthen your wording.
Context: ...ctively. ### Code contributions We are always looking for ways to make SMACT better a...(ALWAYS_CONSTANTLY)
Markdownlint
README.md
127-127: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
125-125: null
Bare URL used(MD034, no-bare-urls)
125-125: null
Bare URL used(MD034, no-bare-urls)
Additional comments not posted (5)
requirements.txt (4)
39-39: Updated dependency: multiprocessThe version of
multiprocesshas been incremented from0.70.15to0.70.16. This appears to be a minor update, likely involving bug fixes or small improvements. Verify that this update does not introduce any issues with existing multiprocessing implementations in the project.
35-35: Updated dependency: montyThe version of
montyhas been updated significantly from2023.11.3to2024.7.30. Ensure that this version is compatible with other dependencies and the project's current codebase.
115-116: Added dependency: typing-extensionsThe addition of
typing-extensions==4.12.2suggests an enhancement in type hinting capabilities, possibly for compatibility with newer Python features. Confirm that this addition aligns with the project's Python version and coding standards.Verification successful
Compatibility of
typing-extensions==4.12.2with Python 3.9+The project requires Python version 3.9 or higher, as specified in
setup.py. Thetyping-extensions==4.12.2package is compatible with Python 3.9 and above, according to its release notes and PyPI page. Therefore, the addition aligns with the project's requirements and standards. No issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for compatibility of typing-extensions with the project's Python version. # Test: Search for compatibility notes for typing-extensions 4.12.2. rg --type markdown -A 5 $'typing-extensions 4.12.2 compatibility'Length of output: 66
Script:
#!/bin/bash # Search for the Python version specified in common configuration files. rg --type toml 'python' -A 3 rg --type py 'python_requires' -A 3Length of output: 121
Script:
#!/bin/bash # Check for compatibility notes for typing-extensions 4.12.2 in the repository. rg 'typing-extensions' -A 5Length of output: 535
75-75: Updated dependency: pymatgenThe version of
pymatgenhas been updated from2023.11.12to2024.5.1. Given the importance of this package in materials science projects, it's crucial to ensure that the new version integrates well with the project's modules that depend onpymatgen.README.md (1)
133-133: Reference to contributing guide addedA new line directing contributors to the
CONTRIBUTING.mdguide has been added. This is a positive change as it provides clear guidance on how to contribute to the project. Ensure that theCONTRIBUTING.mdfile is up-to-date and accessible.Verification successful
Reference to contributing guide verified
The
CONTRIBUTING.mdfile exists in the repository, confirming the review comment's suggestion to ensure its accessibility. The file is present and accessible as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and accessibility of the CONTRIBUTING.md file. # Test: Check if the CONTRIBUTING.md file exists and is accessible. fd 'CONTRIBUTING.md'Length of output: 36
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- README.md (1 hunks)
Additional context used
LanguageTool
README.md
[style] ~128-~128: Consider a different adverb to strengthen your wording.
Context: ...ctively. ### Code contributions We are always looking for ways to make SMACT better a...(ALWAYS_CONSTANTLY)
Markdownlint
README.md
127-127: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
125-125: null
Bare URL used(MD034, no-bare-urls)
125-125: null
Bare URL used(MD034, no-bare-urls)
Additional comments not posted (1)
README.md (1)
133-133: Reference to the contributing guide added.The addition of a reference to the contributing guide in the "Code contributions" section is a useful update for potential contributors. It directs them to more detailed information on how to contribute effectively.
The code changes are approved.
| @@ -122,14 +122,15 @@ Development notes | |||
| ----------------- | |||
|
|
|||
| ### Bugs, features and questions | |||
| Please use the [Issue Tracker](https://github.com/WMD-group/smact/issues) to report bugs or request features in the first instance. While we hope that most questions can be answered by searching [the docs](https://smact.readthedocs.io/en/latest/), we welcome new questions on the issue tracker, especially if they helps us improve the docs! For other queries about any aspect of the code, please contact either Dan Davies (author) or Anthony Onwuli (maintainer) by e-mail: d.w.davies@imperial.ac.uk or [email protected] respectively. | |||
| Please use the [Issue Tracker](https://github.com/WMD-group/smact/issues) to report bugs or request features in the first instance. While we hope that most questions can be answered by searching [the docs](https://smact.readthedocs.io/en/latest/), we welcome new questions on the issue tracker, especially if they help us improve the docs! For other queries about any aspect of the code, please contact either Aron Walsh on behalf of The SMACT Developers (author) or Anthony Onwuli (maintainer) by e-mail: a.walsh@imperial.ac.uk or [email protected] respectively. | |||
There was a problem hiding this comment.
Correct grammatical error in the issue tracker sentence.
The previous comment by coderabbitai[bot] highlighted a grammatical error that still needs correction. The verb form should be 'help' instead of 'helps' to agree with the plural subject 'they'.
Apply this diff to correct the grammar:
- especially if they helps us improve the docs!
+ especially if they help us improve the docs!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.
| Please use the [Issue Tracker](https://github.com/WMD-group/smact/issues) to report bugs or request features in the first instance. While we hope that most questions can be answered by searching [the docs](https://smact.readthedocs.io/en/latest/), we welcome new questions on the issue tracker, especially if they help us improve the docs! For other queries about any aspect of the code, please contact either Aron Walsh on behalf of The SMACT Developers (author) or Anthony Onwuli (maintainer) by e-mail: [email protected] or [email protected] respectively. | |
| Please use the [Issue Tracker](https://github.com/WMD-group/smact/issues) to report bugs or request features in the first instance. While we hope that most questions can be answered by searching [the docs](https://smact.readthedocs.io/en/latest/), we welcome new questions on the issue tracker, especially if they help us improve the docs! For other queries about any aspect of the code, please contact either Aron Walsh on behalf of The SMACT Developers (author) or Anthony Onwuli (maintainer) by e-mail: [email protected] or [email protected] respectively. |
Tools
Markdownlint
125-125: null
Bare URL used(MD034, no-bare-urls)
125-125: null
Bare URL used(MD034, no-bare-urls)
Merging develop branch updates into master branch
Description
N/A
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Test suite updates
Test Configuration:
Reviewers
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Elementclass with additional properties for more detailed elemental information.smact_filterandsmact_validityfunctions with updated default settings for oxidation states.Bug Fixes
Tests
Documentation