refactor: ♻️ Cleanup code into seperate functions #1183
Conversation
Also add `best_tol` parameter to CIReport class
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Refactoring
PR Summary: This pull request introduces a refactor of the CIReport class within the spectrafit project. The changes include the addition of a new parameter best_tol to allow for configurable tolerance in determining the 'BEST' label for confidence intervals. Additionally, the refactor breaks down the report generation process into smaller, more focused methods such as calculate_offset and create_report_row, enhancing the modularity and readability of the code.
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.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Ensure that the new
best_tolparameter is clearly documented, especially its impact on determining the 'BEST' label for confidence intervals. This will help future maintainers and users understand its purpose and how to use it effectively. - Consider revising the condition in the
create_report_rowmethod to handle negative values ofcvalcorrectly by usingabs(cval) < self.best_tol. This change will ensure that the method accurately identifies values close to zero as 'BEST', regardless of their sign.
spectrafit/report.py: The refactor introduces unnecessary complexity through additional methods and parameters; consider simplifying by integrating new features more directly into existing logic.
While the intention behind the refactor to introduce best_tol and modularize the code with additional methods like calculate_offset and create_report_row is appreciated for its aim to improve readability and maintainability, it inadvertently increases the overall complexity of the class. This is primarily due to the introduction of more parameters, conditionals, and loops, which elevates the cognitive load required to understand and navigate the code.
A more streamlined approach could achieve similar improvements in code clarity and functionality without significantly adding to its complexity. For instance, integrating best_tol directly within the existing logic, while retaining a more linear flow, could simplify understanding and maintaining the code. Below is a suggested revision that incorporates best_tol effectively, yet maintains a simpler structure by minimizing the introduction of new methods and conditionals:
class ConfidenceReport:
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 convp(self, x, bound_type):
return "BEST" if abs(x[0]) < self.best_tol else f"{x[0] * 100:.2f}% - {bound_type}"
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"
report.setdefault(self.convp((cval, val), bound_type), {})[name] = sval
self.df = pd.DataFrame(report)
self.tabulate(df=self.df)
def tabulate(self, df):
PrintingResults.print_tabulate_df(df=df, floatfmt=f".{self.ndigits}f")This approach aims to balance the introduction of new functionality with maintaining the simplicity and readability of the code, ensuring it remains easy to navigate and modify in the future.
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? ✨
| str: The confidence interval as a string. | ||
| """ | ||
| return ( | ||
| "BEST" if abs(x[0]) < self.best_tol else f"{x[0] * 100:.2f}% - {bound_type}" |
There was a problem hiding this comment.
suggestion (llm): Using self.best_tol in the convp method to determine the 'BEST' label dynamically is a significant improvement. It makes the method more adaptable to different precision requirements. However, ensure that the documentation or comments clearly explain the impact of best_tol on the 'BEST' label determination to avoid any confusion for future maintainers or users.
| offset (float): The offset for the row. | ||
| """ | ||
| for i, (cval, val) in enumerate(row): | ||
| sval = val if cval < self.best_tol else val - offset |
There was a problem hiding this comment.
issue (llm): The condition cval < self.best_tol in the create_report_row method might not correctly handle cases where cval is negative. Consider using abs(cval) < self.best_tol to ensure that both positive and negative values of cval close to zero are treated as 'BEST'.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1183 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 44 44
Lines 4462 4467 +5
=========================================
+ Hits 4462 4467 +5
|
Also add
best_tolparameter to CIReport classAll PR-Submissions:
Pull Requests for the same
update/change?
New ✨✨ Feature-Submissions:
Changes to ⚙️ Core-Features:
us to include them?