Skip to content
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

refactor: similar item #1417

Merged
merged 4 commits into from
Oct 13, 2024
Merged

refactor: similar item #1417

merged 4 commits into from
Oct 13, 2024

Conversation

weibullguy
Copy link
Collaborator

@weibullguy weibullguy commented Oct 13, 2024

Does this PR introduce a breaking change?

  • Yes
  • No

Refactored calculate_user_defined() Function

  • Looped Calculation of Results (res1, res2, ..., res5):
  • Replaced the repetitive individual assignments of res1, res2, ..., res5 with a single loop.
  • Dynamically generated the keys for equations (equation1, equation2, ..., equation5) and results.
  • Used the sympify() function from SymPy to evaluate each equation and assign its result to the corresponding res key in the sia dictionary.
  • Added a check to set results to 0.0 if the corresponding equation is empty or missing.

Benefits of the Refactor:

  • Reduced Code Duplication: The loop eliminates repeated code, making the function shorter and more maintainable.
  • Improved Readability: Dynamic key generation makes the function easier to understand by following a consistent structure.
  • Error Handling: The function now gracefully handles empty or missing equations by defaulting results to 0.0.

Pull Request Checklist

  • Code Style

    • Code is following code style guidelines.
  • Static Checks

    • Failing static checks are only applicable to code outside the scope of
      this PR.
  • Tests

    • At least one test for all newly created functions/methods?
  • Chores

    • Issue(s) have been raised for problem areas outside the scope of
      this PR. These problem areas have been decorated with an ISSUE: # comment.

Summary by Sourcery

Refactor the calculate_user_defined function to eliminate code duplication and improve maintainability by using loops and helper functions. Enhance test coverage with additional test cases for edge scenarios. Update pre-commit configuration to adjust linting rules.

Enhancements:

  • Refactor the calculate_user_defined function to reduce code duplication by using a loop for dynamic key generation and result calculation.
  • Introduce helper functions _do_update_sia_values, _do_validate_equation, and _do_validate_temperature to improve code readability and maintainability.
  • Replace repetitive temperature conversion logic with a dedicated _do_validate_temperature function.

Tests:

  • Update tests to use pytest.approx for floating-point comparisons to improve test accuracy.
  • Add new test cases for invalid temperature input and handling of empty equations in calculate_user_defined.

Chores:

  • Update pre-commit configuration to ignore specific ruff linting rule PLR2004.

Copy link
Contributor

sourcery-ai bot commented Oct 13, 2024

Reviewer's Guide by Sourcery

This pull request refactors the calculate_user_defined() function in the Similar Item Analysis module, improving code readability, maintainability, and error handling. The changes also include updates to related functions and tests to accommodate the refactoring.

Class diagram for refactored calculate_user_defined function

classDiagram
    class SimilarItemAnalysis {
        +Dict~str, int|float|str~ calculate_user_defined(Dict~str, int|float|str~ sia)
        +Dict~str, float~ calculate_topic_633(Dict~str, float~ temperature, ...)
        +Dict~str, float~ set_user_defined_change_factors(Dict~str, float~ sia, List~float~ factors)
        +Dict~str, float~ set_user_defined_floats(Dict~str, float~ sia, List~float~ floats)
        +Dict~str, int~ set_user_defined_ints(Dict~str, int~ sia, List~int~ ints)
        +Dict~str, str~ set_user_defined_functions(Dict~str, str~ sia, List~str~ functions)
        +Dict~str, float~ set_user_defined_results(Dict~str, float~ sia, List~float~ results)
        +void _do_update_sia_values(Dict~str, Any~ sia, List~Any~ values, int start_idx, int end_idx, Any default_value, Type value_type)
        +str _do_validate_equation(str equation)
        +float _do_validate_temperature(float temp)
    }
Loading

File-Level Changes

Change Details Files
Refactored calculate_user_defined() function to use a loop for calculating results
  • Replaced repetitive assignments with a single loop
  • Dynamically generated keys for equations and results
  • Used sympify() function from SymPy to evaluate equations
  • Added check to set results to 0.0 for empty or missing equations
  • Updated function signature to accept Dict[str, int
float
Introduced helper functions to improve code organization and reduce duplication
  • Added _do_update_sia_values() function to update SIA dictionary values
  • Created _do_validate_equation() function to validate and return equations
  • Implemented _do_validate_temperature() function to validate and round temperatures
src/ramstk/analyses/similaritem.py
Updated related functions to use new helper functions
  • Modified set_user_defined_change_factors() to use _do_update_sia_values()
  • Updated set_user_defined_floats() to use _do_update_sia_values()
  • Changed set_user_defined_ints() to use _do_update_sia_values()
  • Refactored set_user_defined_functions() to use _do_update_sia_values()
  • Updated set_user_defined_results() to use _do_update_sia_values()
src/ramstk/analyses/similaritem.py
Updated tests to accommodate refactored functions and improved assertions
  • Added pytest.approx() to floating-point comparisons for better precision handling
  • Added new tests for invalid temperature input and empty equations
tests/analyses/test_similaritem.py
Updated pre-commit configuration
  • Added --ignore PLR2004 to ruff linter configuration
.pre-commit-config.yaml

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@github-actions github-actions bot added the type: refactor Issue or PR dealing with refactoring of RAMSTK code. label Oct 13, 2024
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @weibullguy - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

if (
_equation_key in sia and sia[_equation_key]
): # Check if equation exists and is not empty
sia[_result_key] = sympify(sia[_equation_key]).evalf(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (bug_risk): Add error handling for equation evaluation

The use of sympify and evalf without error handling could potentially raise uncaught exceptions if an invalid equation is provided. Consider adding try-except blocks to handle potential errors gracefully.

try:
    sia[_result_key] = sympify(sia[_equation_key]).evalf(
        subs={_symbols_dict[j]: sia[_symbol_list[j]]
        for j in range(len(_symbol_list))
        if _symbol_list[j] in sia}
    )
except (SympifyError, ValueError) as e:
    sia[_result_key] = None
    logging.error(f"Error evaluating equation: {e}")

@weibullguy weibullguy merged commit c9ee157 into master Oct 13, 2024
20 of 22 checks passed
@trafico-bot trafico-bot bot added the endgame: merged Pull Request has been merged successfully label Oct 13, 2024
@weibullguy weibullguy deleted the refactor/similaritem branch October 13, 2024 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
endgame: merged Pull Request has been merged successfully type: refactor Issue or PR dealing with refactoring of RAMSTK code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant