Skip to content

power factor calculator - #11727

Open
ShoaibAbbasDev wants to merge 3 commits into
TheAlgorithms:masterfrom
ShoaibAbbasDev:Power-factor-calculator
Open

power factor calculator#11727
ShoaibAbbasDev wants to merge 3 commits into
TheAlgorithms:masterfrom
ShoaibAbbasDev:Power-factor-calculator

Conversation

@ShoaibAbbasDev

@ShoaibAbbasDev ShoaibAbbasDev commented Oct 4, 2024

Copy link
Copy Markdown

Describe your change:

  • [:heavy_check_mark: ] Add an algorithm?
  • [:heavy_check_mark: ] Fix a bug or typo in an existing algorithm?
  • Add or change doctests? -- Note: Please avoid changing both code and tests in a single pull request.
  • Documentation change?

Checklist:

  • [:white_check_mark: ] I have read CONTRIBUTING.md.
  • [ ✅ ] This pull request is all my own work -- I have not plagiarized.
  • [:heavy_check_mark: ] I know that pull requests will not be merged if they fail the automated tests.
  • [:heavy_check_mark: ] This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • [:heavy_check_mark: ] All new Python files are placed inside an existing directory.
  • [:heavy_check_mark: ] All filenames are in all lowercase characters with no spaces or dashes.
  • [:heavy_check_mark: ] All functions and variable names follow Python naming conventions.
  • [:heavy_check_mark: ] All function parameters and return values are annotated with Python type hints.
  • [:heavy_check_mark: ] All functions have doctests that pass the automated testing.
  • [ ✔️ ] All new algorithms include at least one URL that points to Wikipedia or another similar explanation.
  • [:heavy_check_mark: ] If this pull request resolves one or more open issues then the description above includes the issue number(s) with a closing keyword: "Fixes #ISSUE-NUMBER".

@ShoaibAbbasDev
ShoaibAbbasDev force-pushed the Power-factor-calculator branch from d86f7cd to 451ac39 Compare October 4, 2024 06:05

@priya-sundaram-dev priya-sundaram-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, @ShoaibAbbasDev! The power-factor-correction maths look right. A few things needed before this can be merged, matching the repo's CONTRIBUTING conventions:

  1. Type hints are required on every function. e.g. def calculate_apparent_power(voltage: float, current: float) -> float:, and likewise for the others.
  2. Doctests are required — right now the module has none, so CI's doctest requirement isn't satisfied. Please add >>> examples to each function (including the error paths, e.g. calculate_power_factor(100, 0) raising ValueError), and a if __name__ == "__main__": import doctest; doctest.testmod() block.
  3. The try/except TypeError -> raise ValueError wrapping is an anti-pattern here. With float type hints you don't need to guard against non-numbers, and calculate_reactive_power/calculate_correction_capacitance catch ValueError only to re-raise it with a "Calculation error:" prefix, which double-wraps and obscures the original message. I'd drop the try/except and validate inputs directly (if apparent_power == 0: raise ValueError(...)).

Also worth cross-checking against #9535 (shunt-capacitor PF correction) and #12356 so the electronics PF/transform modules don't overlap.

@cclauss cclauss added require tests Tests [doctest/unittest/pytest] are required require type hints https://docs.python.org/3/library/typing.html labels Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

require tests Tests [doctest/unittest/pytest] are required require type hints https://docs.python.org/3/library/typing.html

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants