refactor: Remove magic numbers in ScienceLab.temperature#268
refactor: Remove magic numbers in ScienceLab.temperature#268Architrb1795 wants to merge 2 commits intofossasia:developfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the ScienceLab.temperature property to replace hardcoded CTMU calibration magic numbers with a centralized class-level calibration table and adds explicit error handling for unsupported current-source values, while preserving the existing temperature calculation behavior. Class diagram for updated ScienceLab temperature calibrationclassDiagram
class ScienceLab {
+_CTMU_TEMPERATURE_CALIBRATION: dict[int, tuple[float, float]]
+temperature() float
+_get_ctmu_voltage(channel: int, current_range: int, tgen: bool) float
}
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 - I've left some high level feedback:
- Consider adding a type annotation to
_CTMU_TEMPERATURE_CALIBRATION(e.g.,dict[int, tuple[float, float]]) to make the expected key/value types explicit and help with static analysis. - Since
csis effectively an enum of supported current sources, you might improve readability by introducing named constants or anEnumfor the allowed current source values instead of using bare integers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a type annotation to `_CTMU_TEMPERATURE_CALIBRATION` (e.g., `dict[int, tuple[float, float]]`) to make the expected key/value types explicit and help with static analysis.
- Since `cs` is effectively an enum of supported current sources, you might improve readability by introducing named constants or an `Enum` for the allowed current source values instead of using bare integers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hi maintainers 👋 Happy to adjust the approach if you’d prefer a different structure. |
There was a problem hiding this comment.
Pull request overview
Refactors ScienceLab.temperature to centralize CTMU temperature calibration constants, aiming to improve readability and reduce inline “magic numbers” in the temperature calculation.
Changes:
- Added a class-level
_CTMU_TEMPERATURE_CALIBRATIONmapping for CTMU calibration parameters. - Updated
temperatureto compute via calibration lookup and raise a clear error for unsupported values.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pslab/sciencelab.py
Outdated
| cs = 3 | ||
| V = self._get_ctmu_voltage(0b11110, cs, 0) | ||
|
|
||
| if cs == 1: | ||
| return (646 - V * 1000) / 1.92 # current source = 1 | ||
| elif cs == 2: | ||
| return (701.5 - V * 1000) / 1.74 # current source = 2 | ||
| elif cs == 3: | ||
| return (760 - V * 1000) / 1.56 # current source = 3 | ||
| try: | ||
| offset, slope = self._CTMU_TEMPERATURE_CALIBRATION[cs] | ||
| except KeyError as exc: | ||
| msg = f"Unsupported CTMU current source: {cs}" | ||
| raise ValueError(msg) from exc | ||
| return (offset - V * 1000) / slope |
There was a problem hiding this comment.
PR description says hardcoded numeric literals were removed from ScienceLab.temperature, but the method still contains several unexplained literals (cs = 3, the CTMU channel 0b11110, and the * 1000 unit conversion). Consider extracting these into named constants (or adding a brief comment explaining the units/meaning) so the "remove magic numbers" goal is fully met.
pslab/sciencelab.py
Outdated
| # Calibration parameters for CTMU temperature measurement. | ||
| # Format: {current_source: (offset, slope)} | ||
| _CTMU_TEMPERATURE_CALIBRATION = { | ||
| 1: (646, 1.92), | ||
| 2: (701.5, 1.74), | ||
| 3: (760, 1.56), |
There was a problem hiding this comment.
The calibration mapping comment uses the term current_source, but the value used as the key (cs) is passed as current_range to _get_ctmu_voltage (documented as {0,1,2,3} ranges). This mismatch is confusing—please rename the comment/format description (or the local variable) to consistently reflect what the keys represent (e.g., current_range).
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Thanks for the PR. Centralizing the CTMU calibration constants is a good cleanup and the lookup plus explicit error for unsupported values improves maintainability.
Before we can merge, please do the following:
-
Open an issue first and link it from the PR description using GitHub best practice, for example
Fixes #<issue-number>. This helps track why we are changing calibration logic and what behavior we expect. -
Please align naming and remove remaining ambiguity. The calibration mapping comment refers to “current source”, but the key is passed as
current_rangeto_get_ctmu_voltage. Please rename the comment and or the local variable so it is consistent and clear. -
The “remove magic numbers” goal is only partial.
cs = 3,0b11110, and* 1000are still unexplained. Either extract them into named constants or add brief comments describing what they represent and the units. -
Add proof of behavior. Please attach a short screencast showing this running on your machine, including the exact command. Minimum:
toxorpytestrun passing. Ideally also show a tiny snippet printingScienceLab().temperaturewith your setup, so we can confirm the code path is exercised.
Once these are in place, this becomes a clean refactor we can merge confidently.
|
Small process note. We have automatic Copilot PR reviews enabled on this repository. These reviews are only triggered if the contributor has GitHub Copilot enabled and an active license on their own account. Please enable Copilot in your GitHub settings if you have access. In many regions, free licenses are available through educational institutions or developer programs. Enabling Copilot helps us speed up the auto review process and reduces manual review overhead for the core team. The team will review your PR. Thank you for your contribution and cooperation. |
2 participants
Fixes #272
Summary
This PR addresses the
# TODO: Get rid of magic numbersinScienceLab.temperatureby extracting calibration constants into anamed class-level dictionary.
Changes
_CTMU_TEMPERATURE_CALIBRATIONtemperatureproperty to use a dictionary lookupVerification
tox -e lintSummary by Sourcery
Refactor CTMU-based MCU temperature measurement to use named calibration constants instead of hardcoded literals, adding validation for unsupported current sources.
Enhancements: