-
Notifications
You must be signed in to change notification settings - Fork 7
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: 🌱 cleanup report and test #1182
Conversation
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.
PR Type: Enhancement
PR Summary: This pull request introduces a new class, CIReport, to generate and print a report of confidence intervals for a given set of parameters. It refactors the existing reporting functionality by removing direct calls to the tabulate
function and instead utilizing a more structured approach to generate the report. Additionally, it introduces a best_tol
parameter to the CIReport class to specify the tolerance for determining the best value.
Decision: Comment
📝 Type: 'Enhancement' - 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:
- Consider consolidating the documentation for class constructors to avoid redundancy and ensure consistency across the codebase.
- Review the handling of default values and conditional logic within methods to ensure clarity and maintainability.
- Evaluate the coupling between the new CIReport class and existing printing functionality to enhance flexibility and adaptability for future changes.
spectrafit/report.py: The code change introduces unnecessary complexity through additional methods and parameters, impacting maintainability.
While the introduction of best_tol
and the refactoring into methods like calculate_offset
and create_report_row
aim to enhance modularity and readability, it inadvertently increases the overall complexity of the code. This complexity comes from additional parameters, increased cognitive load due to jumping between methods to follow the logic, and more conditional checks that could potentially introduce subtle bugs.
A more streamlined approach could be to minimize the number of methods and keep the logic more localized within the __call__
method. This would reduce the cognitive load and make the code easier to follow and maintain. Here's a simplified version that incorporates best_tol
while aiming to keep the code concise:
class Report:
def __init__(self, ci, with_offset=True, ndigits=5, best_tol=1.0e-2):
self.ci = ci
self.with_offset = with_offset
self.ndigits = ndigits
self.best_tol = best_tol
self.df = pd.DataFrame()
def __call__(self):
report = {}
for name, row in self.ci.items():
offset = 0.0
if self.with_offset:
for cval, val in row:
if abs(cval) < self.best_tol:
offset = val
break # Assuming we only need the first match
for i, (cval, val) in enumerate(row):
sval = val if cval < self.best_tol else val - offset
bound_type = "LOWER" if i < len(row) / 2 else "UPPER"
conv_key = "BEST" if abs(cval) < self.best_tol else f"{cval * 100:.2f}% - {bound_type}"
report.setdefault(conv_key, {})[name] = sval
self.df = pd.DataFrame(report)
self.tabulate(self.df)
def tabulate(self, df):
PrintingResults.print_tabulate_df(df=df, floatfmt=f".{self.ndigits}f")
This version aims to balance between modularity and simplicity, making the code easier to understand and maintain.
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? ✨
Defaults to 5. | ||
""" | ||
|
||
def __init__( |
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.
suggestion (llm): The CIReport
class constructor's documentation repeats the argument descriptions from the class docstring. Consider removing these from the constructor docstring to avoid redundancy and potential discrepancies in future updates.
float: The offset for the row. | ||
""" | ||
offset = 0.0 | ||
if self.with_offset: |
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.
suggestion (llm): The check for self.with_offset
inside calculate_offset
method implies that if with_offset
is False
, the offset will always be 0.0
. This behavior is correct but consider if there's a more explicit way to handle the offset calculation to improve readability and maintainability.
|
||
self.tabulate(df=pd.DataFrame(self.report)) | ||
|
||
def tabulate(self, df: pd.DataFrame) -> None: |
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.
suggestion (llm): The tabulate
method in CIReport
class is tightly coupled with the PrintingResults.print_tabulate_df
static method. Consider if there's a way to make this method more flexible or generalized, in case the printing or formatting requirements change in the future.
Also add `best_tol` parameter to CIReport class
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1182 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 44 44
Lines 4462 4467 +5
=========================================
+ Hits 4462 4467 +5
|
c0f7c82
to
f839fff
Compare
Quality Gate passedIssues Measures |
Pull request was closed
Also add
best_tol
parameter to CIReport classAll PR-Submissions:
Pull Requests for the same
update/change?
New ✨✨ Feature-Submissions:
Changes to ⚙️ Core-Features:
us to include them?