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

Feature/consts #326

Merged
merged 5 commits into from
Feb 3, 2024
Merged

Feature/consts #326

merged 5 commits into from
Feb 3, 2024

Conversation

blooop
Copy link
Collaborator

@blooop blooop commented Feb 3, 2024

No description provided.

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.

PR Type: Refactoring

PR Summary: The pull request introduces changes to the way variables for input, result, and constants are handled within the benchmarking system. It removes the previous sweep method and seems to be part of a larger refactor to streamline the handling of these variables, potentially in preparation for integrating them with a SweepCfg configuration class.

Decision: Comment

📝 Type: 'Refactoring' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
  • Unsupported files: the diff contains files that Sourcery does not currently support during reviews.

General suggestions:

  • Ensure that the removal of the sweep method and the introduction of input_vars, result_vars, and const_vars do not introduce any regressions or unintended behavior changes.
  • Consider initializing input_vars, result_vars, and const_vars to empty lists or providing appropriate checks before their usage to prevent potential AttributeErrors.
  • Refactor the repeated conditional checks for None values into a private method to reduce code duplication and improve maintainability.
  • Verify that the logic for plotting results is correctly implemented after the removal of the plot variable from the condition, as this could change when and how results are plotted.
  • Address the potential NameError that could occur due to the use of the plot variable, which is no longer in the method signature.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

# self.bench_cfg = BenchCfg()

# Maybe put this in SweepCfg
self.input_vars = None
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (llm): Initializing input_vars, result_vars, and const_vars to None without any further initialization or checks could lead to AttributeError when they are accessed later on. Consider initializing them to empty lists if the logic allows, or ensure there are checks in place before their usage.

@@ -291,18 +270,27 @@
logging.info(
"No input variables passed, using all param variables in bench class as inputs"
)
input_vars = self.worker_class_instance.get_inputs_only()
if self.input_vars is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (llm): The conditional checks for self.input_vars, self.result_vars, and self.const_vars being None are repeated multiple times. Consider refactoring this logic into a private method to reduce code duplication and improve maintainability.

@@ -447,7 +441,7 @@

bench_res.post_setup()

if plot and bench_res.bench_cfg.auto_plot:
if bench_cfg.auto_plot:
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (llm): The check for bench_cfg.auto_plot replaces the previous plot and bench_res.bench_cfg.auto_plot. Ensure that the removal of the plot variable from the condition does not alter the intended behavior, as it seems to change the logic for when plotting should occur.

@@ -392,7 +380,13 @@
title=title,
pass_repeat=pass_repeat,
tag=run_cfg.run_tag + tag,
auto_plot=plot,
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (llm): The auto_plot parameter is being set to plot, but the plot parameter is no longer part of the method signature. This will raise a NameError. Ensure that the correct variable is being used here.

Copy link

codecov bot commented Feb 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6424876) 90.17% compared to head (a8df444) 90.24%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #326      +/-   ##
==========================================
+ Coverage   90.17%   90.24%   +0.07%     
==========================================
  Files          50       50              
  Lines        2870     2912      +42     
==========================================
+ Hits         2588     2628      +40     
- Misses        282      284       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blooop blooop merged commit 28d75c5 into main Feb 3, 2024
6 checks passed
@blooop blooop deleted the feature/consts branch February 3, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant