-
Notifications
You must be signed in to change notification settings - Fork 230
Fix: Correct indentation in analog.py and Resolve SPI test TypeError. #265
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors analog signal calibration internals by documenting and slightly restructuring the _calibrate helper and the waveform range normalization helper for analog outputs, without changing their core numerical behavior. Class diagram for updated analog calibration and waveform normalizationclassDiagram
class AnalogChannel {
- str _name
- float _gain
- int _resolution
- np.poly1d _scale
- np.poly1d _unscale
- tuple RANGE
- np.ndarray _waveform_table
+ resolution(value int) void
- _calibrate() void
+ lowres_waveform_table() np.ndarray
- _range_normalize(x np.ndarray, norm int) np.ndarray
}
class INPUT_RANGES {
}
AnalogChannel --> INPUT_RANGES : uses
class np_poly1d {
}
AnalogChannel --> np_poly1d : sets_scale_and_unscale
class numpy_ndarray {
}
AnalogChannel --> numpy_ndarray : uses_waveform_table
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've found 1 issue, and left some high level feedback:
- The
_calibratemethod has lost one level of indentation and is no longer aligned with the other methods on the class; as written it will be defined at the wrong scope and won’t be available as an instance method. _range_normalizehas also been dedented out of the class whilelowres_waveform_tablestill callsself._range_normalize, which will raise an error at runtime—either re-indent it as a method or adjust the call site accordingly.- The implementation changes in
analog.pydon’t match the pull request description about fixing a SPI test frequency calculation; double-check that you’ve committed the intended file changes for this PR.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_calibrate` method has lost one level of indentation and is no longer aligned with the other methods on the class; as written it will be defined at the wrong scope and won’t be available as an instance method.
- `_range_normalize` has also been dedented out of the class while `lowres_waveform_table` still calls `self._range_normalize`, which will raise an error at runtime—either re-indent it as a method or adjust the call site accordingly.
- The implementation changes in `analog.py` don’t match the pull request description about fixing a SPI test frequency calculation; double-check that you’ve committed the intended file changes for this PR.
## Individual Comments
### Comment 1
<location> `pslab/instrument/analog.py:146` </location>
<code_context>
self._resolution = 2**value - 1
self._calibrate()
- def _calibrate(self):
- A = INPUT_RANGES[self._name][0] / self._gain
- B = INPUT_RANGES[self._name][1] / self._gain
</code_context>
<issue_to_address>
**issue (bug_risk):** Fix the indentation of `_calibrate` to match the surrounding methods.
`_calibrate` is indented with three spaces instead of the four used for other methods, which can cause misalignment or an indentation error. Please align `def _calibrate(...)` with the other methods (four leading spaces) and indent the docstring consistently under it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Description
The
test_spi.pytests were failing during collection becausePWM_FERQUENCYwas attempting to perform arithmetic on theSPIMaster._frequencymethod itself, rather than its return value.Changes
PWM_FERQUENCY.masterfixture.()to correctly call the_frequencymethod.Summary by Sourcery
Document and clarify analog calibration and waveform normalization behavior in the analog instrument module.
Enhancements:
Documentation: