-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chloe lee doper #264
Chloe lee doper #264
Conversation
…ss to store all possible dopants and use all candidates for plotting.
WalkthroughThe updates encompass several areas: adding the "tabulate" module to the configuration for documentation, updating the packaging to include a new data directory and refining the dependency for 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Outside diff range and nitpick comments (4)
setup.py (3)
Line range hint
14-14
: Remove unused import:unittest
.- import unittest
Line range hint
16-16
: Remove unused import:setuptools.Extension
.- from setuptools import Extension
Line range hint
25-25
: Use a context manager for opening files to ensure proper resource management.- long_description=open(os.path.join(module_dir, "README.md")).read(), + with open(os.path.join(module, "README.md"), "r") as f: + long_description=f.read(),smact/dopant_prediction/doper.py (1)
Line range hint
316-316
: Rename unused loop control variabledopant_type
to_dopant_type
to indicate it is intentionally unused.- for dopant_type, dopants in self.results.items(): + for _dopant_type, dopants in self.results.items():Tools
Ruff
3-3:
typing.Type
imported but unused (F401)Remove unused import:
typing.Type
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- docs/conf.py (1 hunks)
- setup.py (1 hunks)
- smact/dopant_prediction/doper.py (13 hunks)
- smact/tests/test_doper.py (2 hunks)
Files not summarized due to errors (2)
- smact/tests/test_doper.py: Error: Server error. Please try again later.
- smact/dopant_prediction/doper.py: Error: Server error. Please try again later.
Files skipped from review due to trivial changes (1)
- docs/conf.py
Additional context used
Ruff
setup.py
14-14:
unittest
imported but unused (F401)Remove unused import:
unittest
16-16:
setuptools.Extension
imported but unused (F401)Remove unused import:
setuptools.Extension
25-25: Use context handler for opening files (SIM115)
smact/tests/test_doper.py
4-4:
smact
imported but unused (F401)Remove unused import:
smact
6-6:
smact.structure_prediction.mutation
imported but unused (F401)Remove unused import:
smact.structure_prediction.mutation
14-14: Local variable
num_dopants
is assigned to but never used (F841)Remove assignment to unused variable
num_dopants
smact/dopant_prediction/doper.py
3-3:
typing.Type
imported but unused (F401)Remove unused import:
typing.Type
316-316: Loop control variable
dopant_type
not used within loop body (B007)Rename unused
dopant_type
to_dopant_type
Additional comments not posted (1)
setup.py (1)
46-46
: Addition of new data directory for JSON files.This inclusion will ensure that the new JSON files are packaged with the software, enhancing the functionality related to species representation.
smact/tests/test_doper.py
Outdated
|
||
class dopant_prediction_test(unittest.TestCase): | ||
|
||
class DopantPredictionTest(unittest.TestCase): | ||
def test_dopant_prediction(self): | ||
num_dopants = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove assignment to unused variable num_dopants
.
- num_dopants = 10
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.
num_dopants = 10 |
Tools
Ruff
14-14: Local variable
num_dopants
is assigned to but never used (F841)Remove assignment to unused variable
num_dopants
smact/tests/test_doper.py
Outdated
@@ -1,11 +1,15 @@ | |||
import os | |||
import unittest | |||
|
|||
import smact | |||
from smact.dopant_prediction import doper | |||
from smact.structure_prediction import mutation, utilities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused import: smact.structure_prediction.mutation
.
- from smact.structure_prediction import mutation
Committable suggestion was skipped due to low confidence.
Tools
Ruff
6-6:
smact.structure_prediction.mutation
imported but unused (F401)Remove unused import:
smact.structure_prediction.mutation
smact/tests/test_doper.py
Outdated
@@ -1,11 +1,15 @@ | |||
import os | |||
import unittest | |||
|
|||
import smact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused import: smact
.
- import smact
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.
import smact |
Tools
Ruff
4-4:
smact
imported but unused (F401)Remove unused import:
smact
smact/dopant_prediction/doper.py
Outdated
from itertools import groupby | ||
from typing import Callable, List, Tuple, Type, Union | ||
from typing import List, Optional, Tuple, Type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused import: typing.Type
.
- from typing import List, Optional, Tuple, Type
+ from typing import List, Optional, Tuple
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.
from typing import List, Optional, Tuple, Type | |
from typing import List, Optional, Tuple |
Tools
Ruff
3-3:
typing.Type
imported but unused (F401)Remove unused import:
typing.Type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
smact/dopant_prediction/doper.py (1)
Line range hint
319-319
: Rename unused loop control variabledopant_type
to_dopant_type
.- for dopant_type, dopants in self.results.items(): + for _dopant_type, dopants in self.results.items():
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- smact/dopant_prediction/doper.py (4 hunks)
- smact/tests/test_doper.py (3 hunks)
Additional context used
Ruff
smact/tests/test_doper.py
26-26: Local variable
dopants
is assigned to but never used (F841)Remove assignment to unused variable
dopants
61-61: Local variable
dopants
is assigned to but never used (F841)Remove assignment to unused variable
dopants
smact/dopant_prediction/doper.py
319-319: Loop control variable
dopant_type
not used within loop body (B007)Rename unused
dopant_type
to_dopant_type
Additional comments not posted (3)
smact/tests/test_doper.py (1)
1-1
: The test methodtest_format_number
is correctly implemented and covers its intended functionality.smact/dopant_prediction/doper.py (2)
302-305
: The refactoredget_dopants
method correctly returns the top results as intended.
1-1
: The_format_number
method is correctly implemented and handles number formatting as expected.
@@ -59,7 +58,8 @@ def test_dopant_prediction_skipspecies(self): | |||
test = doper.Doper( | |||
test_specie, embedding="skipspecies", use_probability=False | |||
) | |||
result = test.get_dopants() | |||
dopants = test.get_dopants() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove assignment to unused variable dopants
.
- dopants = test.get_dopants()
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.
dopants = test.get_dopants() |
Tools
Ruff
61-61: Local variable
dopants
is assigned to but never used (F841)Remove assignment to unused variable
dopants
@@ -25,7 +23,8 @@ | |||
_, an_charge = utilities.parse_spec(anion_min_charge) | |||
|
|||
# Assert: Length of the list and return type (dictionary: list) | |||
result = test.get_dopants() | |||
dopants = test.get_dopants() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove assignment to unused variable dopants
.
- dopants = test.get_dopants()
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.
dopants = test.get_dopants() |
Tools
Ruff
26-26: Local variable
dopants
is assigned to but never used (F841)Remove assignment to unused variable
dopants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
setup.py (3)
Line range hint
14-14
: Remove unused import to clean up the code.- import unittest
Line range hint
16-16
: Remove unused import to clean up the code.- from setuptools import Extension
Line range hint
25-25
: Use a context manager to handle file operations safely and efficiently.- long_description=open(os.path.join(module_dir, "README.md")).read(), + with open(os.path.join(module_dir, "README.md"), 'r') as file: + long_description=file.read(),
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- setup.py (1 hunks)
Additional context used
Ruff
setup.py
14-14:
unittest
imported but unused (F401)Remove unused import:
unittest
16-16:
setuptools.Extension
imported but unused (F401)Remove unused import:
setuptools.Extension
25-25: Use context handler for opening files (SIM115)
Additional comments not posted (1)
setup.py (1)
53-53
: Ensure compatibility with future versions by specifying an upper limit on the numpy version.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #264 +/- ##
===========================================
- Coverage 74.50% 74.49% -0.02%
===========================================
Files 24 24
Lines 2083 2082 -1
===========================================
- Hits 1552 1551 -1
Misses 531 531 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
smact/dopant_prediction/doper.py (1)
Line range hint
319-319
: Unused loop control variable.The loop variable
dopant_type
is not used within the loop body, which can be misleading. Consider renaming it to_
if its value is not needed.- for dopant_type, dopants in self.results.items(): + for _, dopants in self.results.items():
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- examples/Dopant_Prediction/doper_example.ipynb (2 hunks)
- smact/dopant_prediction/doper.py (4 hunks)
Additional context used
Ruff
smact/dopant_prediction/doper.py
319-319: Loop control variable
dopant_type
not used within loop bodyRename unused
dopant_type
to_dopant_type
(B007)
Additional comments not posted (6)
examples/Dopant_Prediction/doper_example.ipynb (3)
144-144
: Clarification on default colormap.The added note about the default color scheme 'YlOrRd' is helpful for users. It enhances the documentation by guiding users on how to change the colormap if needed.
159-159
: Use of updated colormap parameter.The change to use the 'Reds' colormap in the
plot_dopants
method is consistent with the PR's aim to enhance visualisation features. Ensure that this change is reflected in any related documentation or examples to maintain consistency.
221-221
: Update Python version in metadata.Updating the Python version in the notebook metadata from '3.9.16' to '3.9.7' should be verified to ensure compatibility with all dependencies and the overall environment setup.
Verification successful
Python version update verified.
The update of the Python version in the notebook metadata to '3.9.7' is compatible with the repository's setup, as the
setup.py
specifiespython_requires=">=3.9"
and no conflicting dependency constraints were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Python version compatibility with dependencies. # Test: List Python version requirements from setup.py and compare with notebook version. rg --type py 'python_requires' setup.pyLength of output: 72
Script:
#!/bin/bash # Description: Verify Python version compatibility with dependencies. # Test: List all dependencies and their required versions from setup.py or equivalent files. rg --type py 'install_requires' setup.py rg --type py 'requirements' requirements.txtLength of output: 112
Script:
#!/bin/bash # Description: Verify Python version compatibility with dependencies. # Test: Search for dependency specifications in other common configuration files. rg --type toml 'dependencies' pyproject.toml rg --type toml 'requires' Pipfile rg --type yaml 'dependencies' conda.ymlLength of output: 216
smact/dopant_prediction/doper.py (3)
3-3
: Maintain import consistency.The import statement for
List, Optional, Tuple
fromtyping
is correctly maintained. This is crucial for type hinting throughout the module, enhancing code readability and maintainability.
302-305
: Refinement inget_dopants
return logic.The refactoring to return the top
num_dopants
results for each category using list slicing and dictionary comprehension is a clean and efficient approach. This change improves the readability and performance of the method.
Line range hint
307-338
: Enhancements inplot_dopants
method.The adjustments to the color mapping and the handling of the heatmap plotting are well-implemented. However, ensure that the
assert
statement for checking if dopants are calculated is effective and that it doesn't interrupt user workflows negatively.
Pull Request Template
Description
Now "Doper" class stores all possible dopants, and returns top n candidates as a return of "get_dopants" method. This is to use all candidates in plotting the periodic table heatmap with "plot_dopants" method.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Reviewers
@mention individuals who you specifically want to involve in the discussion for this pull request and mention why they are needed in the discussion/why they are needed to review the pull request.
Checklist:
Summary by CodeRabbit
Documentation
New Features
species_rep
directory in the package.Bug Fixes
doper_example.ipynb
.Refactor
get_dopants
function to return a modified dictionary with top results.plot_dopants
function.Tests
DopantPredictionTest
.Chores
numpy
to require a version less than 2.