Added shunt capacitor power factor correction - #9535
Conversation
for more information, see https://pre-commit.ci
removed unnecessary pass and reformatted long line
for more information, see https://pre-commit.ci
|
Seems correct but can you don't add L and R also with another function, all 3 in a single class??? |
|
Ok but Resistors don't affect reactive power inductors and capacitors do |
|
For R, to correct the power factor of a load with an undesired resistance, I think u need to add a parallel shunt resistor, its calculation will involve the real power and resistance. It's been 2 yrs since I don't touched electrical engineering, so correct me if I goes wrong somewhere. |
|
Thats possible sir |
done |
|
@priya-sundaram-dev, your review, please. Is there any code cleanup that we should consider? |
priya-sundaram-dev
left a comment
There was a problem hiding this comment.
Thanks for the review request, @cclauss. The math is sound and the doctests (including the ValueError traceback cases) are a nice touch. A few cleanup items, roughly in priority order:
1. Docstring copy-paste bug (should fix). shunt_inductor_power_factor_correction still says "Calculate shunt capacitor...". It should describe the inductor, and ideally state the returned unit (farads for the capacitor, henries for the inductor) plus a one-line note on sign conventions.
2. Heavy duplication (main cleanup opportunity). The two functions are ~90% identical — same validation block and the same four reactive-power lines. That's the thing I'd most want to DRY up. A small private helper keeps each public function to its final formula, e.g.:
def _reactive_power_delta(voltage, frequency, real_power, pf_now, pf_target):
for pf in (pf_now, pf_target):
if not isinstance(pf, (int, float)) or not -1 <= pf <= 1:
raise ValueError("power_factor must be a valid float between -1 and 1.")
if frequency == 0:
raise ValueError("frequency is zero dc circuit")
if voltage == 0:
raise ValueError("voltage is zero no excitation")
q_now = (real_power / pf_now) * math.sin(math.acos(pf_now))
q_target = (real_power / pf_target) * math.sin(math.acos(pf_target))
return q_now - q_targetThen the capacitor returns delta / (2*pi*f*V**2) and the inductor V**2 / (2*pi*f*delta).
3. Comment style. The mid-function """...""" blocks are string-expression statements, not comments — please switch them to # comments so they don't read as unused expressions.
4. chr/bool edge case. isinstance(True, (int, float)) is True, so pf=True sneaks through as 1. Minor; a bool guard is optional but tidy.
5. Inductor division-by-zero. The inductor formula puts change_reactive_power in the denominator, so pf_now == pf_target raises ZeroDivisionError. Worth an explicit ValueError (or a doctest) since the capacitor version can't hit that.
6. Chained comparison. -1 <= pf <= 1 reads cleaner than the four-way or chain (ruff-friendly too).
None of these are blockers — 1 and 2 are the ones I'd ask for before merge; 3–6 are polish. Happy to push a follow-up commit with the helper extraction if the author would like a hand.
priya-sundaram-dev
left a comment
There was a problem hiding this comment.
Reviewed @cclauss — the math is sound (Q = P·(tanφ₁ − tanφ₂), then C = ΔQ/(ω·V²) for the capacitor, L = V²/(ω·ΔQ) for the inductor) and both doctests reproduce. A few cleanups worth making before merge:
-
Copy-paste docstring —
shunt_inductor_power_factor_correction's summary still says "Calculate shunt capacitor…". Should read "inductor". -
Unguarded division by zero —
current_apparent_power = real_power / current_power_factorraisesZeroDivisionErrorwhencurrent_power_factor == 0(a purely reactive load, which is exactly when correction matters most). The-1..1range check lets0through. Suggest rejecting0explicitly, e.g.if current_power_factor == 0 or expected_power_factor == 0: raise ValueError("power factor cannot be zero"), with a doctest covering it. -
Dead string literals used as comments — the triple-quoted blocks inside each function body (
"""The difference between the new and old reactive powers…""") are expression statements, not docstrings (they aren't the first statement). Please convert them to#comments so they don't sit in the compiled code. -
Duplication (optional) — the two functions share identical validation + reactive-power computation; only the final return differs. A small private
_reactive_power_change(...)helper would remove ~20 duplicated lines, but I'm fine leaving it if you'd rather keep each function self-contained for readability.
Items 1–3 are worth fixing; 4 is a nice-to-have. Nice addition to electronics/ otherwise.
Describe your change:
Checklist: