-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add environ
module and refactor unit handling
#96
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces an Sequence diagram for SI mode computationsequenceDiagram
participant C as Client Code
participant E as Environ Module
participant Q as Quantity
C->>E: context(compute_mode='si')
activate E
E->>Q: Convert quantities to SI units
Q->>Q: factorless()
Q->>Q: Adjust mantissa
Q-->>E: Return SI quantities
E-->>C: Execute in SI context
deactivate E
Class diagram for environ moduleclassDiagram
class DefaultContext {
+Dict settings
+defaultdict contexts
+Dict functions
}
class EnvironModule {
+SI_MODE: str
+NON_SI_MODE: str
+set(compute_mode: str)
+get(key: str): Any
+context(**kwargs): ContextManager
+get_compute_mode(): str
+all(): dict
}
DefaultContext --* EnvironModule: uses
State diagram for compute mode transitionsstateDiagram-v2
[*] --> NON_SI_MODE: Default
NON_SI_MODE --> SI_MODE: set(compute_mode='si')
SI_MODE --> NON_SI_MODE: set(compute_mode='non_si')
NON_SI_MODE --> NON_SI_MODE: context(compute_mode='non_si')
SI_MODE --> SI_MODE: context(compute_mode='si')
state SI_MODE {
[*] --> ConvertUnits
ConvertUnits --> AdjustMantissa
AdjustMantissa --> UseFactorless
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
convert_in_si
Function
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 @Routhleck - I've reviewed your changes - here's some feedback:
Overall Comments:
- The
convert_in_si()
function modifies global state by changing variables in the caller's scope. Consider adding a warning in the docstring about potential side effects and thread-safety concerns when using this function.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue 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.
convert_in_si
Functionenviron
module to control computation in SI or non-SI mode
@sourcery-ai review |
environ
module to control computation in SI or non-SI modeThere 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 @Routhleck - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more comprehensive tests for the environ module, particularly around error cases and nested context manager usage
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue 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.
@@ -1808,7 +1809,7 @@ def __mul__(self, other) -> 'Unit' | Quantity: | |||
dim, | |||
scale=scale, | |||
base=self.base, |
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.
issue (bug_risk): Bug: Using self.factor instead of the calculated factor variable could lead to incorrect unit conversions
The factor parameter should use the local factor variable that was calculated earlier in the method, not self.factor
environ
module and remove commented-out code
environ
module and remove commented-out codeenviron
module and refactor unit handling
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…in/brainunit into compute-in-standard-unit
Summary by Sourcery
Introduce the
environ
module to enable switching between SI and non-SI computation modes. Remove commented-out code blocks for enhanced readability and maintainability.New Features:
environ
module to control computation in SI or non-SI units.Tests:
environ
module and computation modes.