Add Typecheck Utils and mark all type errors #20
Conversation
Reviewer's Guide by SourceryThis PR adds type checking annotations and fixes type errors throughout the codebase. The changes primarily involve adding type ignore comments and explicit type annotations to resolve mypy type checking issues. Class diagram for Vec class with type annotationsclassDiagram
class Vec {
+float x
+float y
+float z
+to_rosetta() xyzVector_double_t
+dot(Vec v) float
+normdot(Vec v) float
+angle(Vec v) float
+angle_degrees(Vec v) float
+lineangle(Vec v) float
+lineangle_degrees(Vec v) float
+length() float
+length_squared() float
+distance(Vec v) float
+distance_squared(Vec v) float
+cross(Vec v) Vec
+normalize() void
+normalized() Vec
+outer(Vec v) Mat
+unit() Vec
+abs() Vec
+tuple() tuple
+key() tuple
+round0() void
}
Class diagram for Mat class with type annotationsclassDiagram
class Mat {
+float xx
+float xy
+float xz
+float yx
+float yy
+float yz
+float zx
+float zy
+float zz
+to_rosetta() xyzMatrix_double_t
+row(int i) Vec
+col(int i) Vec
+rowx() Vec
+rowy() Vec
+rowz() Vec
+colx() Vec
+coly() Vec
+colz() Vec
+transpose() void
+transposed() Mat
+det() float
+trace() float
+add_diagonal(Vec v) Mat
+is_rotation() bool
}
Class diagram for Xform class with type annotationsclassDiagram
class Xform {
+Mat R
+Vec t
+from_four_points(Vec cen, Vec a, Vec b, Vec c) Xform
+from_two_vecs(Vec a, Vec b) Xform
+tolocal(Vec x) Vec
+toglobal(Vec x) Vec
+inverse() Xform
+rotation_axis() Vec
+rotation_axis_center() Vec
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @willsheffler - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Using eval() is a security risk - consider using a safer alternative or restoring safe_eval (link)
Overall Comments:
- Consider gradually replacing some of the type: ignore comments with proper type annotations in follow-up PRs to improve type safety while keeping the code working
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🔴 Security: 1 blocking issue
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 4 issues found
- 🟢 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.
| @@ -257,14 +294,18 @@ def apply_symmetry_index(self, idx: T, val, isidx, **kw) -> T: | |||
| assert th.allclose(newidx, idx) | |||
| return new | |||
|
|
|||
| def apply_symmetry_scalar(self, shapekind: ShapeKind, contig: 'th.Tensor', **kw) -> 'th.Tensor': | |||
| N = len(contig) // self.nsub | |||
| def apply_symmetry_scalar(self, shapekind: ShapeKind, contig: 'th.Tensor', **kw) -> 'th.Tensor': # type: ignore | |||
There was a problem hiding this comment.
issue (bug_risk): Potential division by zero error if self.nsub is 0
Add validation to ensure self.nsub is non-zero before performing integer division with //. This could cause runtime crashes.
|
|
||
| def get_all_annotations(cls): | ||
| annotations = {} | ||
| for base in cls.__mro__[::-1]: | ||
| annotations |= getattr(base, '__annotations__', {}) | ||
| return annotations | ||
|
|
||
| def fstr(template): | ||
| return safe_eval(f'f"""{template}"""') | ||
| def eval_fstring(template, namespace): |
There was a problem hiding this comment.
🚨 issue (security): Using eval() is a security risk - consider using a safer alternative or restoring safe_eval
The eval() function can execute arbitrary Python code which could be malicious. Even with a restricted namespace, it can still access builtins unless explicitly prevented.
| raise Valuerror(f'cant make hypothesis strat for {T} {type(T)} {orig} {args}') | ||
| if (strat := self.type_mapping.get(T, None)) is not None: return strat # type: ignore | ||
|
|
||
| raise Valuerror(f'cant make hypothesis strat for {T} {type(T)} {orig} {args}') # type: ignore |
There was a problem hiding this comment.
issue (typo): Typo in error name 'Valuerror' - should be 'ValueError'
| SymElem(2, axis=[1, 0, 0], cen=[0.25, 0.5, 0.0], hel=0.5, label='C21'), # type: ignore | ||
|
|
||
| SymElem(2, axis=[1, 0, 0], cen=[0.25, 0.5, 0.5], hel=0.5, label='C21'), # type: ignore | ||
|
|
||
| ], | ||
| ) | ||
| helper_test_symelem('P622', val, debug, **kw) |
There was a problem hiding this comment.
suggestion (testing): Test data validation could be improved
The test data for space group symmetry elements contains many hardcoded values without clear validation of their correctness. Consider adding assertions or documentation explaining how these reference values were determined and why they are correct.
expected_result = get_reference_symelems('P622')
assert_symelems_match(expected_result, val, 'P622')
helper_test_symelem('P622', val, debug, **kw)| assert b3.d == "dee" | ||
|
|
||
| foo = Namespace(a=1, b="c") | ||
| b = Bunch(foo, _strict=False) | ||
| b = Bunch(foo, _strict=False) # type: ignore | ||
|
|
||
| assert b.a == 1 | ||
| assert b.b == "c" | ||
| assert b.missing is None |
There was a problem hiding this comment.
suggestion (testing): Missing edge case tests for Bunch initialization
The test_bunch_init() function could be expanded to cover more edge cases like empty dictionaries, None values, nested bunches, and invalid input types. This would help ensure the Bunch class handles all scenarios correctly.
def test_bunch_init():
b = Bunch(dict(a=2, b="bee"), _strict=False)
b2 = Bunch(b, _strict=False)
b3 = Bunch(c=3, d="dee", _strict=False, **b)
b4 = Bunch({}, _strict=False)
b5 = Bunch(dict(x=None), _strict=False)
b6 = Bunch(dict(y={"z": 1}), _strict=False)
assert b.a == 2 and b.b == "bee"
assert b3.d == "dee" and b3.a == 2
assert b4.missing is None
assert b5.x is None
assert b6.y.z == 1|
|
||
| sg_symelem_frame444_compids_dict = sgdata.get('sg_symelem_frame444_compids_dict', dict()) # type: ignore | ||
|
|
||
| sg_symelem_frame444_opcompids_dict = sgdata.get('sg_symelem_frame444_opcompids_dict', dict()) # type: ignore |
There was a problem hiding this comment.
suggestion (code-quality): Replace dict() with {} (dict-literal)
| sg_symelem_frame444_opcompids_dict = sgdata.get('sg_symelem_frame444_opcompids_dict', dict()) # type: ignore | |
| sg_symelem_frame444_opcompids_dict = sgdata.get('sg_symelem_frame444_opcompids_dict', {}) # type: ignore |
Explanation
The most concise and Pythonic way to create a dictionary is to use the{}notation.
This fits in with the way we create dictionaries with items, saving a bit of
mental energy that might be taken up with thinking about two different ways of
creating dicts.
x = {"first": "thing"}Doing things this way has the added advantage of being a nice little performance
improvement.
Here are the timings before and after the change:
$ python3 -m timeit "x = dict()"
5000000 loops, best of 5: 69.8 nsec per loop
$ python3 -m timeit "x = {}"
20000000 loops, best of 5: 29.4 nsec per loop
Similar reasoning and performance results hold for replacing list() with [].
| if len(cperr.match) >= len(bestbadiframes): # type: ignore | ||
|
|
||
| if np.max(cperr.match) < np.max(bestbadiframes): # type: ignore | ||
|
|
||
| bestbadiframes, bestbadelem = cperr.match, movedelem # type: ignore |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if len(cperr.match) >= len(bestbadiframes): # type: ignore | |
| if np.max(cperr.match) < np.max(bestbadiframes): # type: ignore | |
| bestbadiframes, bestbadelem = cperr.match, movedelem # type: ignore | |
| if len(cperr.match) >= len(bestbadiframes) and np.max(cperr.match) < np.max(bestbadiframes): | |
| bestbadiframes, bestbadelem = cperr.match, movedelem # type: ignore | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
| for i in range(9): | ||
| assert np.allclose(SS(diffuse=i).linear1, i + 1) | ||
| assert np.allclose(SS(diffuse=i).linear1, i + 1) # type: ignore |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| for i in range(9): | ||
| assert np.allclose(SS(rfold=i).square, i**2) | ||
| assert np.allclose(SS(rfold=i).square, i**2) # type: ignore |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| for i in range(9): | ||
| assert np.allclose(SS(design=i).cube, i**3) | ||
| assert np.allclose(SS(design=i).cube, i**3) # type: ignore |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Summary by Sourcery
Enhancements: