feat: Add future annotations and update version to 1.4.4 #124
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Updated package version in __init__.py - Added future annotations import in tools.py and cmd.py - Refactored calculation method signature in batch.py - Added new dependencies in pyproject.toml and updated poetry.lock
|
Review changes with SemanticDiff. Analyzed 8 of 10 files. Overall, the semantic diff is 30% smaller than the GitHub diff.
|
Reviewer's Guide by SourceryThis pull request introduces future annotations for improved type hinting, updates the package version to 1.4.4, and enhances the codebase with better typing and code structure. The changes primarily focus on improving type annotations, refactoring class structures, and updating method signatures across multiple files in the tanabesugano package. Class diagram for LigandFieldTheory and its subclassesclassDiagram
class LigandFieldTheory {
+Float64Array eigensolver(Float64Array matrix)
+Dict~str, Float64Array~ solver()
}
class d2 {
+Float64Array A_1_1_states()
+Float64Array E_1_states()
+Float64Array T_1_2_states()
+Float64Array T_3_1_states()
}
class d3 {
+Float64Array T_2_2_states()
+Float64Array T_2_1_states()
+Float64Array E_2_states()
+Float64Array T_4_1_states()
}
class d4 {
+Float64Array T_3_1_states()
+Float64Array T_1_2_states()
+Float64Array A_1_1_states()
+Float64Array E_1_1_states()
+Float64Array T_3_2_states()
+Float64Array T_1_1_states()
+Float64Array E_3_1_states()
+Float64Array A_3_2_states()
+Float64Array A_1_2_states()
}
class d5 {
+Float64Array T_2_2_states()
+Float64Array T_2_1_states()
+Float64Array E_2_states()
+Float64Array A_2_1_states()
+Float64Array A_2_2_states()
+Float64Array T_4_1_states()
+Float64Array T_4_2_states()
+Float64Array E_4_states()
}
class d6 {
+Float64Array T_3_1_states()
+Float64Array T_1_2_states()
+Float64Array A_1_1_states()
+Float64Array E_1_1_states()
+Float64Array T_3_2_states()
+Float64Array T_1_1_states()
+Float64Array E_3_1_states()
+Float64Array A_3_2_states()
+Float64Array A_1_2_states()
}
class d7 {
+Float64Array T_2_2_states()
+Float64Array T_2_1_states()
+Float64Array E_2_states()
+Float64Array T_4_1_states()
}
class d8 {
+Float64Array A_1_1_states()
+Float64Array E_1_states()
+Float64Array T_1_2_states()
+Float64Array T_3_1_states()
}
LigandFieldTheory <|-- d2
LigandFieldTheory <|-- d3
LigandFieldTheory <|-- d4
LigandFieldTheory <|-- d5
LigandFieldTheory <|-- d6
LigandFieldTheory <|-- d7
LigandFieldTheory <|-- d8
Class diagram for CMDmainclassDiagram
class CMDmain {
+float Dq
+float B
+float C
+int nroot
+DataFrame df
+void plot()
+void label_plot(str arg0, str arg1, str arg2)
+void savetxt()
+void calculation()
+dict subsplit_states(dict states)
+void ci_cut(float dq_ci)
+void ts_print(dict states, float dq_ci)
+void interactive_plot()
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Phylum OSS Supply Chain Risk Analysis - FAILEDThis repository analyzes the risk of new dependencies. An If you see this comment, one or more dependencies have failed Phylum's risk analysis. Package:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Anselmoo - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| """ | ||
| return eigh(matrix)[0] | ||
|
|
||
| def solver(self) -> Dict[str, Float64Array]: |
There was a problem hiding this comment.
suggestion: Standardize solver method return type across all subclasses
Ensure that all subclasses of LigandFieldTheory implement the solver method with the same return type annotation. This will improve consistency and make the code more maintainable.
def solver(self) -> Dict[str, np.ndarray]:
| ) | ||
| else: | ||
| raise ValueError("`d_count` must be in {2,3,4,5,6,7,8}") | ||
| msg = "The number of unpaired electrons should be between 2 and 8." |
There was a problem hiding this comment.
suggestion: Improve error handling for invalid d_count values
Consider adding a check for d_count at the beginning of the method or in the init to fail fast and provide a clear error message. This will improve the user experience when invalid inputs are provided.
def __init__(self, d_count):
if not 2 <= d_count <= 8:
raise ValueError("The number of unpaired electrons should be between 2 and 8.")
self.d_count = d_count
# Rest of the class implementation...
| @@ -8,16 +10,16 @@ def state_check(x): | |||
| if x == 3: | |||
| states = matrices.d3(Dq=i).solver() | |||
| return len(states) | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding more comprehensive tests for the state_check function
The current test only checks the number of states returned. It would be beneficial to add tests that verify the correctness of the states themselves, not just their count. Also, consider adding test cases for d2 and d8 configurations.
from __future__ import annotations
import numpy as np
from tanabesugano import matrices
def test_state_check():
assert state_check(3) == 120
assert state_check(2) == 45
assert state_check(8) == 1001
d3_states = matrices.d3(Dq=1000).solver()
assert len(d3_states) == 120
assert isinstance(d3_states[0], np.ndarray)
assert d3_states[0].shape == (7,)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce future annotations for improved type hinting across the codebase. Update the package version to 1.4.4 and add new dependencies. Enhance documentation for dependencies.
Summary by Sourcery
Introduce future annotations for improved type hinting and refactor the codebase to use a new LigandFieldTheory parent class. Update the package version to 1.4.4 and enhance documentation for dependencies.
New Features:
Enhancements:
Documentation: