Adding example with analysis of reinforced concrete columns under biaxial bending #29
Conversation
Reviewer's Guide by SourceryThis pull request adds an example with analysis of reinforced concrete columns under biaxial bending. It introduces new functionality for drawing and analyzing concrete column cross-sections, calculating PMM (axial force-moment-moment) interaction diagrams, and generating calculation reports. The changes include new classes, functions, and tests related to concrete column analysis, as well as updates to existing canvas and table functionality in the efficalc library. Class diagram for new and updated canvas elementsclassDiagram
class CanvasElement {
<<abstract>>
}
class ElementWithMarkers {
+_get_markers() List~Marker~
+get_markers() List~Marker~
+_apply_context_marker_styles(marker: Marker) Marker
}
CanvasElement <|-- ElementWithMarkers
class Line {
+_get_markers() List~Marker~
}
ElementWithMarkers <|-- Line
class Polyline {
+_get_markers() List~Marker~
}
ElementWithMarkers <|-- Polyline
class Text {
+text: str
+x: float
+y: float
+font_size: float | str
+rotate: float
+horizontal_base: Literal["start", "center", "end"]
+vertical_base: Literal["auto", "top", "middle", "bottom"]
+fill: str
+stroke: str
+stroke_width: float
+to_svg() str
}
CanvasElement <|-- Text
class Dimension {
+x1: float
+y1: float
+x2: float
+y2: float
+text: Optional~str~
+gap: float
+offset: float
+unit: Optional~str~
+text_position: Literal["top", "bottom"]
+text_size: float
+to_svg() str
}
ElementWithMarkers <|-- Dimension
class Leader {
+marker_x: float
+marker_y: float
+text_x: float
+text_y: float
+text: str
+marker: Marker
+landing_len: float
+direction: Literal["right", "left"]
+text_size: float
+to_svg() str
}
ElementWithMarkers <|-- Leader
class ArrowMarker {
+reverse: bool
+orientation: MarkerOrientation
+base: Literal["point", "center", "flat"]
+to_svg() str
}
Marker <|-- ArrowMarker
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @janderson4 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more inline documentation for complex algorithms, especially in the PMM search functions. While the code is well-structured, additional comments explaining the mathematical concepts would improve maintainability.
- The new plotting functions using matplotlib and plotly are a great addition. However, consider abstracting the plotting logic further to allow for easier swapping of plotting backends in the future.
Here's what I looked at during the review
- 🟡 General issues: 9 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 5 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
efficalc/canvas/canvas.py
Outdated
| @@ -28,9 +32,11 @@ def __init__( | |||
| self, | |||
| width: float, | |||
| height: float, | |||
| min_xy: tuple[float, float] = (0, 0), | |||
There was a problem hiding this comment.
suggestion: Consider adding a comment explaining the purpose and usage of min_xy
This new parameter affects the canvas coordinate system, but its purpose and effects are not immediately clear. A brief explanation would help users understand how to use it effectively.
min_xy: tuple[float, float] = (0, 0),
"""Minimum x and y coordinates for the canvas. Defaults to (0, 0)."""
efficalc/canvas/canvas_elements.py
Outdated
| self._apply_context_marker_styles(marker) | ||
| return markers | ||
|
|
||
| def _apply_context_marker_styles(self, marker: Marker) -> Marker: |
There was a problem hiding this comment.
suggestion: Consider the placement and implementation of _apply_context_marker_styles
This method seems similar to the one removed from the Canvas class. Consider if this is the optimal location for this functionality or if it could be further simplified.
def _apply_context_marker_styles(self, marker: Marker) -> None:
if marker.fill == "context-fill":
marker.fill = self.context_fill
if marker.stroke == "context-stroke":
marker.stroke = self.context_stroke
efficalc/canvas/canvas_elements.py
Outdated
| def to_svg(self) -> str: | ||
| return f'<path d="{self.to_path_commands()}"{self.get_common_svg_style_elements()}{self._get_marker_assignments()} />' | ||
|
|
||
|
|
||
| class Text(CanvasElement): |
There was a problem hiding this comment.
suggestion: Consider creating a common base class or utility functions for new element classes
The new classes (Text, Dimension, Leader) share similar patterns, especially in their to_svg methods. A common base class or shared utility functions could help reduce code duplication.
class SVGElement(CanvasElement):
def to_svg(self):
raise NotImplementedError("Subclasses must implement to_svg method")
class Text(SVGElement):
efficalc/generate_html.py
Outdated
| @@ -210,6 +214,32 @@ def _generate_comparison_html(item: Comparison) -> str: | |||
| return _wrap_div(comp_html, class_name=CALC_ITEM_WRAPPER_CLASS) | |||
|
|
|||
|
|
|||
| def _generate_result_table_html(item: Table) -> str: | |||
There was a problem hiding this comment.
suggestion: Consider using a template engine or helper functions for HTML generation
The new HTML generation code, particularly in _generate_result_table_html, introduces quite a bit of string concatenation. Consider using a template engine or extracting some of this logic into separate helper functions for better maintainability.
def _generate_result_table_html(item: Table) -> str:
table_attrs = []
if item.full_width:
table_attrs.append('width="100%"')
if item.striped:
table_attrs.append('class="striped"')
return f'<table {" ".join(table_attrs)}>'
| """ | ||
|
|
||
|
|
||
| def plot(pmm_data): |
There was a problem hiding this comment.
suggestion: Consider refactoring the plot function for improved maintainability
The plot function is quite long and complex. Consider breaking it down into smaller, more focused functions to improve readability and maintainability. This will make the code easier to test and modify in the future.
def plot(pmm_data):
"""Generate plots for PMM data."""
return _generate_plots(pmm_data)
def _generate_plots(pmm_data):
X = pmm_data.X
Y = pmm_data.Y
|
|
||
| plt.show() | ||
|
|
||
| # plt.savefig("test_plot.png") |
There was a problem hiding this comment.
suggestion (testing): Visual test for point plotter
This visual test is useful for manually verifying the output of the point plotter. Consider automating this test by comparing the generated plot with a reference image, or by checking specific properties of the plot programmatically.
import matplotlib.pyplot as plt
import numpy as np
from PIL import Image
# ... (rest of the imports and functions remain the same)
if __name__ == "__main__":
# ... (existing code to generate the plot)
plt.savefig("generated_plot.png")
plt.close()
generated_image = np.array(Image.open("generated_plot.png"))
reference_image = np.array(Image.open("reference_plot.png"))
assert np.allclose(generated_image, reference_image, atol=5), "Generated plot differs from reference"
|
|
||
| fig = pmm_plotter_plotly.plot(pmm_data) | ||
|
|
||
| fig.show() |
There was a problem hiding this comment.
suggestion (testing): Visual test for PMM plotter using Plotly
Similar to the point plotter test, consider automating this test. You could check specific properties of the Plotly figure object, such as the number of traces, axis labels, or data ranges.
import pytest
from plotly.graph_objs import Figure
def test_pmm_plotter_plotly():
col = example_col()
axial_limits = assign_max_min.calculate_axial_load_limits(col)
load_data = [[300, 100, 200, True], [-100, 50, -60, False], [1500, 300, -300, False]]
loads = [LoadCombination(*load) for load in load_data]
pmm_data = get_pmm_data.get_pmm_data(col, 36, 12, loads, axial_limits)
fig = pmm_plotter_plotly.plot(pmm_data)
assert isinstance(fig, Figure)
assert len(fig.data) > 0
assert fig.layout.scene.xaxis.title.text == "Mx"
assert fig.layout.scene.yaxis.title.text == "My"
assert fig.layout.scene.zaxis.title.text == "P"
|
|
||
| pmm_data = get_pmm_data.get_pmm_data(example_col, 36, 12, loads, axial_limits) | ||
|
|
||
| _ = pmm_plotter_plotly.plot(pmm_data) |
There was a problem hiding this comment.
suggestion (testing): Basic test for pmm_plotter_plotly function
This test checks for runtime errors but doesn't verify the correctness of the plot. Consider adding assertions to check the structure or content of the generated plot.
from ..calc_document.plotting import get_pmm_data, pmm_plotter_plotly
from ..col import assign_max_min
from ..pmm_search.load_combo import LoadCombination
import plotly.graph_objs as go
def test_pmm_plotter_plotly(example_col, loads):
axial_limits = assign_max_min.calculate_axial_load_limits(example_col)
load_data = [[300, 100, 200, True], [-100, 50, -60, False], [1500, 300, -300, False]]
loads = [LoadCombination(*load) for load in load_data]
pmm_data = get_pmm_data.get_pmm_data(example_col, 36, 12, loads, axial_limits)
fig = pmm_plotter_plotly.plot(pmm_data)
assert isinstance(fig, go.Figure)
assert len(fig.data) > 0
| # in the format (Mx, My, P). | ||
| _, _, _, mesh = pmm_mesh.get_mesh(col, 48, 18, axial_limits) | ||
|
|
||
| _ = get_capacity.get_capacity(mesh, loads[0]) |
There was a problem hiding this comment.
suggestion (testing): Basic test for get_capacity function
This test only checks if the function runs without errors. Consider adding assertions to verify the correctness of the returned capacity values.
from ..calc_document.plotting import get_capacity, pmm_mesh
from ..col.assign_max_min import calculate_axial_load_limits
def test_get_capacity(example_col, loads):
col = example_col
axial_limits = calculate_axial_load_limits(col)
_, _, _, mesh = pmm_mesh.get_mesh(col, 48, 18, axial_limits)
capacity = get_capacity.get_capacity(mesh, loads[0])
assert isinstance(capacity, float)
assert capacity > 0
| _, _, _, mesh = pmm_mesh.get_mesh(example_col3, 36, 12, axial_limits) | ||
|
|
||
| capacity = get_capacity.get_capacity(mesh, loads[0]) | ||
| _ = point_plotter.plot(capacity, loads[0], False) |
There was a problem hiding this comment.
suggestion (testing): Basic test for point_plotter function
Similar to the pmm_plotter_plotly test, this only checks for runtime errors. Consider adding assertions to verify the content or structure of the generated plot.
def test_point_plotter(example_col3, loads):
axial_limits = calculate_axial_load_limits(example_col3)
_, _, _, mesh = pmm_mesh.get_mesh(example_col3, 36, 12, axial_limits)
capacity = get_capacity.get_capacity(mesh, loads[0])
plot = point_plotter.plot(capacity, loads[0], False)
assert isinstance(plot, go.Figure)
assert len(plot.data) > 0
assert plot.layout.title.text is not None
assert plot.layout.xaxis.title.text is not None
assert plot.layout.yaxis.title.text is not None
Make the load case table numbered Factor out the load case column capacities Round very small numbers down to zero Add detail in DCR calculations
Summary by Sourcery
Add a new example for analyzing reinforced concrete columns under biaxial bending, including PMM and PM diagrams, DCR calculations, and detailed calculation reports. Enhance the canvas elements to support markers and improve report generation with new options for displaying long calculations. Update documentation and add comprehensive tests for the new features.
New Features:
Enhancements:
Documentation:
Tests: