Tensile Strength Evaluation as part of Steady State Analysis - #34
Conversation
GitHub Actions
Inactive
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRenames steady‑state API and result types (evaluate_SSERR → evaluate_SteadyState; SSERRResult → MaximalStressResult + new SteadyStateResult), adds per‑layer stress normalization options in Analyzer, refactors NaN/finite handling in plotting, adds plot_visualize_deformation, updates tests, and updates .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Tests/CLI
participant CE as CriteriaEvaluator
participant Sys as SystemModel
participant AN as Analyzer
participant PL as Plotter
Test->>CE: evaluate_SteadyState(system=Sys, ...)
CE->>Sys: read system state
CE->>AN: _calculate_maximal_stresses(system)
AN->>AN: compute Sxx/Txz/Szz (optionally normalize)
AN-->>CE: MaximalStressResult (kPa arrays + norm arrays + maxima)
CE->>CE: compute energy_release_rate
CE-->>Test: SteadyStateResult(..., energy_release_rate, maximal_stress_result, system)
Test->>PL: plot_visualize_deformation(..., analyzer=AN, normalize=...)
PL->>AN: request stresses/displacements (respecting normalize)
AN-->>PL: stress/displacement arrays (finite-masked or normalized)
PL-->>Test: Figure / saved file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/weac/analysis/criteria_evaluator.py (1)
708-711: PotentialAttributeErrorif original system hastouchdown=False.Line 711 accesses
system.slab_touchdown.l_BCon the original system before the copy. Ifsystem.config.touchdownisFalse, thensystem.slab_touchdownisNone(perSystemModel.slab_touchdowncached property), causing anAttributeError.system_copy = copy.deepcopy(system) system_copy.config.touchdown = True system_copy.update_scenario(scenario_config=ScenarioConfig(phi=0.0)) - l_BC = system.slab_touchdown.l_BC + # Ensure touchdown is computed on the copy, not the original + if system_copy.slab_touchdown is None: + raise ValueError("Touchdown analysis failed to initialize.") + l_BC = system_copy.slab_touchdown.l_BCtests/analysis/test_criteria_evaluator.py (1)
186-211: New SteadyState test provides good coverage of the expanded result payloadThis test drives
evaluate_SteadyStatethrough the recommended (vertical=False) path and validates key invariants onenergy_release_rate, touchdown distance, and the normalized maximal stresses, which is a solid sanity/regression check for the newSteadyStateResult/MaximalStressResultAPI. If you later want stronger regression guarantees, you could extend this to assert approximate expected magnitudes or array shapes of the underlying stress fields, but the current level looks appropriate for now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.gitignore(1 hunks)src/weac/analysis/__init__.py(2 hunks)src/weac/analysis/analyzer.py(11 hunks)src/weac/analysis/criteria_evaluator.py(5 hunks)src/weac/analysis/plotter.py(3 hunks)tests/analysis/test_analyzer.py(1 hunks)tests/analysis/test_criteria_evaluator.py(3 hunks)tests/test_regression_simulation.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
tests/test_regression_simulation.py (1)
src/weac/analysis/criteria_evaluator.py (1)
evaluate_SteadyState(679-735)
src/weac/analysis/__init__.py (1)
src/weac/analysis/criteria_evaluator.py (1)
SteadyStateResult(127-153)
tests/analysis/test_analyzer.py (1)
src/weac/analysis/analyzer.py (3)
rasterize_solution(95-176)Sxx(228-291)principal_stress_slab(432-502)
tests/analysis/test_criteria_evaluator.py (1)
src/weac/analysis/criteria_evaluator.py (2)
SteadyStateResult(127-153)evaluate_SteadyState(679-735)
src/weac/analysis/analyzer.py (2)
src/weac/core/system_model.py (2)
z(365-412)fq(146-148)src/weac/core/field_quantities.py (1)
du_dx(64-66)
src/weac/analysis/plotter.py (2)
src/weac/analysis/analyzer.py (6)
Sxx(228-291)Txz(294-359)Szz(362-429)principal_stress_slab(432-502)principal_stress_weaklayer(505-564)get_zmesh(179-225)src/weac/core/field_quantities.py (4)
u(55-62)w(68-70)sig(101-103)tau(105-114)
src/weac/analysis/criteria_evaluator.py (2)
src/weac/core/system_model.py (1)
SystemModel(37-412)src/weac/analysis/analyzer.py (5)
differential_ERR(638-690)Analyzer(50-823)rasterize_solution(95-176)Sxx(228-291)principal_stress_slab(432-502)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (10)
src/weac/analysis/__init__.py (1)
11-11: LGTM! Public API export updated correctly.The import and
__all__export are properly updated to exposeSteadyStateResultinstead of the deprecatedSSERRResult, consistent with the renamed dataclass incriteria_evaluator.py.Also applies to: 21-21
src/weac/analysis/analyzer.py (2)
350-356: Same division-by-zero risk applies here.The same guard for positive
tensile_strengthshould be applied consistently acrossSxx,Txz, andSzznormalization paths.
493-499: LGTM! Unit conversion logic is correct for normalization.The conversion chain properly handles both
kPaandMPaunits: the tensile strength is converted to matchPs's unit before division, yielding a correct dimensionless ratio.tests/analysis/test_analyzer.py (1)
82-116: LGTM! Good test coverage for unit conversion and normalization.The test properly validates:
- Unit conversion relationship (
kPa = MPa * 1e3)- Normalization produces unit-independent (dimensionless) results
This aligns well with the implementation changes in
analyzer.py.src/weac/analysis/plotter.py (2)
888-897: LGTM! NaN filtering correctly applied to weak-layer coordinates.The
nanmaskis properly used to filterxwlbefore meshgrid creation and to slice displacement fields, ensuring consistent array shapes for plotting.
900-949: Consistent NaN masking across all field types.All field computation cases correctly apply
nanmaskor usexwl_finitedimensions, ensuring array shape consistency for contour plotting.src/weac/analysis/criteria_evaluator.py (3)
97-124: LGTM! Well-structured dataclass for maximal stress results.The
MaximalStressResultdataclass clearly separates raw stress values (kPa) from normalized values, and provides convenient scalar maxima. Good design.
126-153: LGTM! Comprehensive steady-state result structure.The
SteadyStateResultdataclass provides a complete evaluation payload including convergence status, energy metrics, stress results, and the modified system model for further analysis.
1212-1241: LGTM! Clean implementation of maximal stress calculation.The helper method correctly computes both raw (kPa) and normalized stress fields, extracts scalar maxima, and packages everything into a
MaximalStressResult. The implementation is consistent with theAnalyzerAPI.tests/analysis/test_criteria_evaluator.py (1)
12-17: SteadyStateResult import matches updated evaluator APIImporting
SteadyStateResultalongside the other result types keeps the tests aligned with the new public API and enables precise type annotations; no issues here.
There was a problem hiding this comment.
Pull request overview
This PR refactors the stress and steady-state evaluation logic by introducing clearer data models and adding tensile strength normalization capabilities. The changes rename SSERR to SteadyState for better clarity and introduce new result classes to encapsulate maximal stress information at touchdown.
Key Changes
- Introduced
MaximalStressResultandSteadyStateResultdataclasses to replaceSSERRResult, providing better structure for stress evaluation results - Added normalization support to
Sxx,Txz,Szz, andprincipal_stress_slabmethods, enabling stress values to be expressed as ratios of tensile strength - Fixed NaN handling in plotting functions to prevent errors when visualizing systems with partially cracked weak layers
Reviewed changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Version bump from 3.0.1 to 3.0.2 |
| tests/test_regression_simulation.py | Updated test to use renamed evaluate_SteadyState method and energy_release_rate attribute |
| tests/analysis/test_criteria_evaluator.py | Updated test names, imports, and assertions for renamed classes/methods; added tests for new maximal stress fields |
| tests/analysis/test_analyzer.py | Added comprehensive tests for stress field unit conversion and normalization behavior |
| src/weac/analysis/criteria_evaluator.py | Added MaximalStressResult dataclass; renamed SSERRResult to SteadyStateResult with enhanced fields; added _calculate_maximal_stresses helper method |
| src/weac/analysis/analyzer.py | Added normalize parameter to Sxx, Txz, Szz methods; fixed unit conversion in principal_stress_slab normalization |
| src/weac/analysis/plotter.py | Improved NaN handling in plot_deformed; added new plot_visualize_deformation method with enhanced coordinate system management |
| src/weac/analysis/init.py | Updated exports to use SteadyStateResult instead of SSERRResult |
| .gitignore | Added dev/ folder to ignore list |
Comments suppressed due to low confidence (3)
src/weac/analysis/analyzer.py:383
- The return type documentation should specify that when
normalize=True, the returned values are dimensionless ratios (stress/tensile_strength) rather than values in the specified unit. For example: "Transverse normal stress at grid points in the slab in specified unit, or dimensionless ratio if normalized."
Returns
-------
ndarray, float
Transverse normal stress at grid points in the slab in
specified unit.
src/weac/analysis/analyzer.py:248
- The return type documentation should specify that when
normalize=True, the returned values are dimensionless ratios (stress/tensile_strength) rather than values in the specified unit. For example: "Axial slab normal stress in specified unit, or dimensionless ratio if normalized."
Returns
-------
ndarray, float
Axial slab normal stress in specified unit.
src/weac/analysis/analyzer.py:314
- The return type documentation should specify that when
normalize=True, the returned values are dimensionless ratios (stress/tensile_strength) rather than values in the specified unit. For example: "Shear stress at grid points in the slab in specified unit, or dimensionless ratio if normalized."
Returns
-------
ndarray
Shear stress at grid points in the slab in specified unit.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/weac/analysis/analyzer.py (1)
284-290: Division by zero risk whentensile_strengthis zero or near-zero.This issue was previously flagged. If any layer has zero or very small
tensile_strength, dividing bytensile_strength_MPawill produceinfor numerically unstable results. The same pattern exists inTxzandSzznormalization blocks.Consider adding validation before normalization:
# Normalize tensile stresses to tensile strength if normalize: tensile_strength_kPa = zmesh["tensile_strength"] tensile_strength_MPa = tensile_strength_kPa / 1e3 + if np.any(tensile_strength_MPa <= 0): + raise ValueError( + "Cannot normalize: tensile_strength must be positive for all layers." + ) # Normalize axial normal stress to layers' tensile strength normalized_Sxx = Sxx_MPa / tensile_strength_MPa[:, None] return normalized_Sxxsrc/weac/analysis/plotter.py (1)
911-925: Bug:normalizeparameter not passed toSxx,Txz,Szzmethods.The
normalizeparameter is accepted byplot_deformedbut is only used for the"principal"case. For"Sxx","Txz", and"Szz"fields, normalization is never applied, and labels always show(kPa)regardless of thenormalizeflag.Compare with
plot_visualize_deformation(lines 1184, 1191, 1196) wherenormalizeis correctly passed.Apply this diff to fix:
# Axial normal stresses (kPa) case "Sxx": - slab = analyzer.Sxx(z, phi, dz=dz, unit="kPa") + slab = analyzer.Sxx(z, phi, dz=dz, unit="kPa", normalize=normalize) weak = np.zeros(xwl_finite.shape[0]) - label = r"$\sigma_{xx}$ (kPa)" + label = r"$\sigma_{xx}/\sigma_+$" if normalize else r"$\sigma_{xx}$ (kPa)" # Shear stresses (kPa) case "Txz": - slab = analyzer.Txz(z, phi, dz=dz, unit="kPa") + slab = analyzer.Txz(z, phi, dz=dz, unit="kPa", normalize=normalize) weak = Tauwl[nanmask] - label = r"$\tau_{xz}$ (kPa)" + label = r"$\tau_{xz}/\sigma_+$" if normalize else r"$\tau_{xz}$ (kPa)" # Transverse normal stresses (kPa) case "Szz": - slab = analyzer.Szz(z, phi, dz=dz, unit="kPa") + slab = analyzer.Szz(z, phi, dz=dz, unit="kPa", normalize=normalize) weak = Sigmawl[nanmask] - label = r"$\sigma_{zz}$ (kPa)" + label = r"$\sigma_{zz}/\sigma_+$" if normalize else r"$\sigma_{zz}$ (kPa)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/weac/analysis/analyzer.py(11 hunks)src/weac/analysis/criteria_evaluator.py(5 hunks)src/weac/analysis/plotter.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/weac/analysis/analyzer.py (2)
src/weac/core/system_model.py (2)
z(365-412)fq(146-148)src/weac/core/field_quantities.py (1)
du_dx(64-66)
src/weac/analysis/criteria_evaluator.py (1)
src/weac/analysis/analyzer.py (4)
differential_ERR(643-695)rasterize_solution(95-176)Sxx(228-293)principal_stress_slab(437-507)
🔇 Additional comments (9)
src/weac/analysis/criteria_evaluator.py (3)
97-124: LGTM! Well-structured dataclass for stress results.The
MaximalStressResultdataclass clearly separates absolute values (kPa) from normalized values, and the docstring adequately documents each field's purpose.
126-154: LGTM! Clean API design for steady-state evaluation results.The
SteadyStateResultdataclass provides a comprehensive result structure including the computed stresses and the system state at touchdown.
726-735: LGTM! Proper integration of maximal stress calculation into steady-state evaluation.The method correctly unpacks the differential ERR and delegates stress computation to the helper method.
src/weac/analysis/analyzer.py (2)
228-246: LGTM! Comprehensive docstring for thenormalizeparameter.The updated documentation clearly explains that when
normalize=True, values are returned as dimensionless ratios and theunitparameter is ignored.
476-507: LGTM! Correct unit conversion for principal stress normalization.The conversion logic properly handles both
kPaandMPaunits:
tensile_strength_kPa / 1e3converts to MPa- Multiplying by
convert[unit]scales back appropriatelyThe principal stress is computed in the requested unit first, then normalized by the tensile strength in the same unit—this maintains dimensional consistency.
src/weac/analysis/plotter.py (4)
888-897: LGTM! Correct NaN handling for weak-layer coordinates.The finite-mask approach properly filters out NaN values from
xwlbefore constructing the mesh grids, preventing plotting errors when weak-layer regions are not fully defined.
1040-1081: LGTM! Well-structured new visualization method.The
plot_visualize_deformationmethod provides a clean normalized coordinate system for visualization, with proper handling of slab and weak-layer proportions. The docstring adequately describes the parameters and return value.
1182-1200: LGTM! Correct label handling for normalized stress fields.Unlike
plot_deformed, this method correctly passesnormalizeto the stress computation methods and updates labels to show dimensionless ratios when normalization is enabled.
1245-1250:xsl[~nanmask]is correct—both arrays have identical shapes by construction.The
nanmaskderived fromxwlcan safely be applied toxslbecause both originate from a singlerasterize_solution()call at line 1698 and are initialized with sizeni.sum()in the function definition. Usingxsl(notxwl) is necessary becausexwlcontains NaN values at cracked positions, whilexslpreserves the coordinate values at those positions for the cracked weak-layer outline plot.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/weac/analysis/criteria_evaluator.py (1)
126-153: SteadyStateResult extension matches new steady-state API; consider clarifying unitsThe added fields (
energy_release_rate,maximal_stress_result,system) make the result richer and consistent with the new evaluation flow. For future readers, it would help to explicitly document the physical units fortouchdown_distanceandenergy_release_ratein the docstring.
♻️ Duplicate comments (2)
src/weac/analysis/plotter.py (2)
1243-1248: Bug:nanmaskderived fromxwlis incorrectly applied toxsl.
nanmaskis computed fromxwl(line 1145), but here it's applied toxsl. Ifxslandxwlhave different shapes or NaN patterns, this will produce incorrect cracked region coordinates or raise anIndexError.Apply this diff to fix:
# Plot cracked weak-layer outline (where there is no weak layer) - xwl_cracked = xsl[~nanmask] + xwl_cracked = xwl[~nanmask] Xwl_cracked, Zwl_cracked_plot = np.meshgrid( 1e-1 * xwl_cracked, [slab_proportion + cracked_proportion, total_height_plot], )
1168-1217: Consider extracting shared field computation logic.The
match fieldblock is nearly identical betweenplot_deformedandplot_visualize_deformation. Extracting this into a helper method would reduce ~50 lines of duplication and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/weac/analysis/criteria_evaluator.py(5 hunks)src/weac/analysis/plotter.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/weac/analysis/criteria_evaluator.py (2)
src/weac/core/system_model.py (1)
SystemModel(37-412)src/weac/analysis/analyzer.py (5)
differential_ERR(643-695)Analyzer(50-828)rasterize_solution(95-176)Sxx(228-293)principal_stress_slab(437-507)
src/weac/analysis/plotter.py (3)
src/weac/analysis/analyzer.py (6)
Sxx(228-293)Txz(296-362)Szz(365-434)principal_stress_slab(437-507)principal_stress_weaklayer(510-569)get_zmesh(179-225)src/weac/core/system_model.py (2)
z(365-412)fq(146-148)src/weac/core/field_quantities.py (4)
u(55-62)w(68-70)sig(101-103)tau(105-114)
🔇 Additional comments (7)
src/weac/analysis/plotter.py (5)
888-897: LGTM!The NaN filtering approach is correct. Filtering
xwlto finite values before constructing the weak-layer grid and displacement fields prevents NaN propagation issues.
902-910: LGTM!The nanmask indexing on
Usl[-1, :]andWsl[-1, :]correctly extracts values corresponding to finitexwlpositions, maintaining consistency with the filtered weak-layer grid.
911-928: LGTM!The labels now correctly reflect the normalization state: dimensionless ratios (
σxx/σ+) whennormalize=True, and units (kPa) whenFalse. This addresses the previous review feedback about incorrect labels.
930-947: LGTM!The principal stress handling correctly filters
weak_fullusingnanmaskto align with finite weak-layer coordinates, and the labels appropriately reflect the normalization state.
972-979: LGTM!The deformed weak-layer outline now uses the consistently-sized
Xwl,Zwl,Uwl, andWwlarrays built from finitexwlvalues.src/weac/analysis/criteria_evaluator.py (2)
97-124: MaximalStressResult dataclass and documentation look consistentThe fields and docstring align with how
_calculate_maximal_stressespopulates principal and axial stresses (both absolute and normalized); no issues from my side here.
151-152: Verifyenergy_release_ratetype hint matchesdifferential_ERRoutput
SteadyStateResult.energy_release_rateis annotated asfloat, while it is populated via:energy_release_rate, _, _ = analyzer.differential_ERR(unit="J/m^2")In other parts of the code,
differential_ERRappears to return arrays that are then indexed (e.g.,diff_energy[1],diff_energy[2]). Please double-check that the first return value here is indeed a scalar steady-state value rather than an array; if it is an array, either:
- adjust the type hint for
energy_release_rate, or- reduce the array to the intended scalar summary before assigning.
Also applies to: 726-727
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/weac/analysis/plotter.py (2)
1116-1120: Consider adding a lower bound forweaklayer_proportion.When
max_w_displacementis very small but non-zero, the calculatedweaklayer_proportioncould become extremely small (close to 0), making the weak layer region invisible. Consider adding a minimum threshold:if weaklayer_proportion is None: if max_w_displacement > 0: - weaklayer_proportion = min(0.3, (h_cm / max_w_displacement) * 0.1) + weaklayer_proportion = max(0.05, min(0.3, (h_cm / max_w_displacement) * 0.1)) else: weaklayer_proportion = 0.3
1170-1219: Significant code duplication withplot_deformed.The
match fieldblock (lines 1171-1219) is nearly identical to the one inplot_deformed(lines 900-947). Consider extracting this into a helper method to reduce duplication:def _compute_field_values(self, field, Usl, Wsl, Sigmawl, Tauwl, nanmask, xwl_finite, analyzer, z, phi, dz, normalize): """Compute field values for slab and weak layer plotting.""" match field: case "u": slab = 1e4 * Usl weak = 1e4 * Usl[-1, nanmask] label = r"$u$ ($\mu$m)" # ... other cases return slab, weak, labelThis would make both plotting methods call the shared helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/weac/analysis/criteria_evaluator.py(5 hunks)src/weac/analysis/plotter.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/weac/analysis/plotter.py (1)
src/weac/analysis/analyzer.py (6)
Sxx(228-293)Txz(296-362)Szz(365-434)principal_stress_slab(437-507)principal_stress_weaklayer(510-569)get_zmesh(179-225)
src/weac/analysis/criteria_evaluator.py (2)
src/weac/core/system_model.py (1)
SystemModel(37-412)src/weac/analysis/analyzer.py (2)
differential_ERR(643-695)Analyzer(50-828)
🔇 Additional comments (8)
src/weac/analysis/plotter.py (4)
888-897: LGTM! NaN handling for weak layer coordinates is well-implemented.The filtering logic correctly:
- Creates
nanmaskfromxwlusingnp.isfinite- Extracts only finite coordinates for meshgrid construction
- Applies the mask consistently to displacement fields
This prevents plotting errors when weak layer coordinates contain NaN values.
911-928: Labels correctly updated for normalization-aware display.The labels now properly reflect dimensionless ratios when
normalize=True(e.g.,r"$\sigma_{xx}/\sigma_+$") and include units whennormalize=False(e.g.,r"$\sigma_{xx}$ (kPa)"). This addresses the inconsistency previously noted.
1038-1081: Docstring is complete and well-documented.The
levelsparameter is now properly documented (line 1068-1069), addressing the previous review comment about the missing parameter documentation.
1245-1258: No defensive check needed for array shape alignment.
xslandxwlare guaranteed to have identical shapes sincerasterize_solutionreturns both as arrays of sizeni.sum(). The shape alignment is by design, not an assumption, so adding an assertion is unnecessary.Likely an incorrect or invalid review comment.
src/weac/analysis/criteria_evaluator.py (4)
98-124: Well-documented dataclass with clear field descriptions.The
MaximalStressResultdataclass properly documents all fields, including both absolute (kPa) and normalized stress values. The naming convention clearly distinguishes between the two forms.
126-154: NewSteadyStateResultdataclass provides comprehensive result encapsulation.The dataclass properly includes:
- Convergence status and message
- Touchdown distance
- Energy release rate (renamed from SSERR for clarity)
- Maximal stress results
- The modified system model
Including the
systemfield is a good design choice as it allows callers to inspect or reuse the modified system state.
726-739:print_call_statsis now correctly honored.The implementation now:
- Forwards
print_call_statsto_calculate_maximal_stresses(line 728)- Calls
analyzer.print_call_stats()when the flag is True (lines 730-731)This addresses the previous review comment about the flag having no visible effect.
1216-1262: Well-implemented helper with proper documentation and explicit mode.The
_calculate_maximal_stressesmethod:
- Has complete docstring with parameters and return type documented
- Explicitly specifies
mode="cracked"inrasterize_solution(line 1238)- Properly invokes
analyzer.print_call_stats()when the flag is True (lines 1251-1254)All previous review concerns have been addressed.
I made a small change and created the issue, so we will remember more in-depth for the future.
Not sure if you will have time, so I will just dismiss the review for now.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/weac/analysis/analyzer.py (2)
284-290: Add guard against division by zero in normalization.The past review comment about division by zero remains unaddressed. If any layer has
tensile_strength≤ 0, the normalization will produceinfor numerically unstable results.Apply a guard before normalization:
# Normalize tensile stresses to tensile strength if normalize: tensile_strength_kPa = zmesh["tensile_strength"] tensile_strength_MPa = tensile_strength_kPa / 1e3 + if np.any(tensile_strength_MPa <= 0): + raise ValueError("Cannot normalize: tensile_strength must be positive for all layers.") # Normalize axial normal stress to layers' tensile strength normalized_Sxx = Sxx_MPa / tensile_strength_MPa[:, None] return normalized_Sxx
425-431: Add guard against division by zero in normalization.Same issue as
SxxandTxz: dividing bytensile_strength_MPawithout checking for zero/negative values.Apply the same guard:
# Normalize tensile stresses to tensile strength if normalize: tensile_strength_kPa = zmesh["tensile_strength"] tensile_strength_MPa = tensile_strength_kPa / 1e3 + if np.any(tensile_strength_MPa <= 0): + raise ValueError("Cannot normalize: tensile_strength must be positive for all layers.") # Normalize transverse normal stress to layers' tensile strength normalized_Szz = Szz_MPa / tensile_strength_MPa[:, None] return normalized_Szzsrc/weac/analysis/plotter.py (2)
1170-1219: Consider extracting shared field computation logic.This
match fieldblock is nearly identical to the one inplot_deformed(lines 900-947). Extract this into a helper method to reduce duplication and improve maintainability.Example helper method:
def _compute_field_values( self, field: str, z: np.ndarray, analyzer: Analyzer, phi: float, dz: int, normalize: bool, Usl: np.ndarray, Wsl: np.ndarray, Sigmawl: np.ndarray, Tauwl: np.ndarray, nanmask: np.ndarray, xwl_finite_shape: int, ) -> tuple[np.ndarray, np.ndarray, str]: """Compute field values for slab and weak layer.""" # ... shared field computation logic return slab, weak, label
1245-1250: Bug:nanmaskapplied to wrong array.
nanmaskis derived fromxwl(line 1147), but line 1246 applies it toxsl. Sincexslandxwlmay have different shapes or NaN patterns, this produces incorrect cracked-region coordinates or dimension misalignment.Fix:
# Plot cracked weak-layer outline (where there is no weak layer) - xwl_cracked = xsl[~nanmask] + xwl_cracked = xwl[~nanmask] Xwl_cracked, Zwl_cracked_plot = np.meshgrid( 1e-1 * xwl_cracked, [slab_proportion + cracked_proportion, total_height_plot], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/weac/analysis/analyzer.py(11 hunks)src/weac/analysis/plotter.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/weac/analysis/analyzer.py (1)
src/weac/core/field_quantities.py (1)
du_dx(64-66)
src/weac/analysis/plotter.py (2)
src/weac/analysis/analyzer.py (6)
Sxx(228-293)Txz(296-362)Szz(365-434)principal_stress_slab(437-507)principal_stress_weaklayer(510-569)get_zmesh(179-225)src/weac/core/field_quantities.py (4)
u(55-62)w(68-70)sig(101-103)tau(105-114)
🔇 Additional comments (3)
src/weac/analysis/analyzer.py (1)
215-217: LGTM: Helpful unit annotation.The comment clarifying that
tensile_strengthis in kPa improves code readability and aligns with the subsequent normalization logic that converts to MPa.src/weac/analysis/plotter.py (2)
888-897: LGTM: Robust NaN handling for weak-layer coordinates.The filtering of NaN values from
xwlbefore creating the meshgrid prevents plotting errors and ensures correct visualization. The displacement fieldsUwlandWwlare properly assembled using the nanmask.
911-928: LGTM: Correctly updated labels for normalized fields.The labels now properly reflect whether normalization is enabled, showing dimensionless ratios (e.g.,
σ_xx/σ_+) whennormalize=Trueor units (e.g.,σ_xx (kPa)) whennormalize=False. This addresses a past review comment.
| # Normalize shear stresses to tensile strength | ||
| if normalize: | ||
| tensile_strength_kPa = zmesh["tensile_strength"] | ||
| tensile_strength_MPa = tensile_strength_kPa / 1e3 | ||
| # Normalize shear stress to layers' tensile strength | ||
| normalized_Txz = Txz_MPa / tensile_strength_MPa[:, None] | ||
| return normalized_Txz |
There was a problem hiding this comment.
Add guard against division by zero and clarify normalization approach.
Two concerns:
-
Division by zero: Same issue as
Sxx- dividing bytensile_strength_MPawithout checking for zero/negative values. -
Normalization approach: Normalizing shear stress by tensile strength (rather than shear strength) is unconventional. If this is intentional for a specific physical reason, please document why in the docstring or add a reference.
Add the same guard as suggested for Sxx:
# Normalize shear stresses to tensile strength
if normalize:
tensile_strength_kPa = zmesh["tensile_strength"]
tensile_strength_MPa = tensile_strength_kPa / 1e3
+ if np.any(tensile_strength_MPa <= 0):
+ raise ValueError("Cannot normalize: tensile_strength must be positive for all layers.")
# Normalize shear stress to layers' tensile strength
normalized_Txz = Txz_MPa / tensile_strength_MPa[:, None]
return normalized_Txz📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Normalize shear stresses to tensile strength | |
| if normalize: | |
| tensile_strength_kPa = zmesh["tensile_strength"] | |
| tensile_strength_MPa = tensile_strength_kPa / 1e3 | |
| # Normalize shear stress to layers' tensile strength | |
| normalized_Txz = Txz_MPa / tensile_strength_MPa[:, None] | |
| return normalized_Txz | |
| # Normalize shear stresses to tensile strength | |
| if normalize: | |
| tensile_strength_kPa = zmesh["tensile_strength"] | |
| tensile_strength_MPa = tensile_strength_kPa / 1e3 | |
| if np.any(tensile_strength_MPa <= 0): | |
| raise ValueError("Cannot normalize: tensile_strength must be positive for all layers.") | |
| # Normalize shear stress to layers' tensile strength | |
| normalized_Txz = Txz_MPa / tensile_strength_MPa[:, None] | |
| return normalized_Txz |
🤖 Prompt for AI Agents
In src/weac/analysis/analyzer.py around lines 353 to 359, the code normalizes
shear stresses by tensile_strength_MPa without guarding against zero or
non-positive values and uses tensile strength for shear normalization without
explanation; add a guard that identifies non-positive tensile strength entries
and handles them (e.g., raise a clear ValueError, skip/NaN those layers, or
substitute a small epsilon) to avoid division-by-zero, and either change the
normalization to use shear strength if that was intended or add a brief
docstring comment and a reference explaining why tensile strength is used for
normalizing shear stresses so the choice is explicit.
| if normalize and val == "max": | ||
| zmesh = self.get_zmesh(dz=dz) | ||
| tensile_strength = zmesh["tensile_strength"] | ||
| tensile_strength_kPa = zmesh["tensile_strength"] | ||
| tensile_strength_converted = tensile_strength_kPa / 1e3 * convert[unit] | ||
| # Normalize maximum principal stress to layers' tensile strength | ||
| normalized_Ps = Ps / tensile_strength[:, None] | ||
| normalized_Ps = Ps / tensile_strength_converted[:, None] | ||
| return normalized_Ps |
There was a problem hiding this comment.
Add division by zero guard and consider simplifying normalization.
Two issues:
-
Division by zero: Missing guard for
tensile_strength_converted <= 0. -
Inconsistent approach: This method computes stress in the requested unit first (lines 486-491), then normalizes. In contrast,
Sxx,Txz, andSzzcompute in MPa, normalize if requested, then convert units. Consider aligning the approaches for consistency.
Add guard:
# Normalize tensile stresses to tensile strength
if normalize and val == "max":
zmesh = self.get_zmesh(dz=dz)
tensile_strength_kPa = zmesh["tensile_strength"]
tensile_strength_converted = tensile_strength_kPa / 1e3 * convert[unit]
+ if np.any(tensile_strength_converted <= 0):
+ raise ValueError("Cannot normalize: tensile_strength must be positive for all layers.")
# Normalize maximum principal stress to layers' tensile strength
normalized_Ps = Ps / tensile_strength_converted[:, None]
return normalized_Ps🤖 Prompt for AI Agents
In src/weac/analysis/analyzer.py around lines 498 to 504, the code normalizes Ps
by tensile_strength_converted but lacks a division-by-zero guard and uses a
different unit-conversion order than Sxx/Txz/Szz; change the logic to compute
tensile strength in MPa first (matching the other methods), apply normalization
against tensile_strength_MPa with a safe guard (e.g., mask or clamp values <= 0
to raise or set to np.inf/np.nan) to avoid division by zero, and then convert
the normalized result to the requested unit—or alternatively clamp
tensile_strength_converted to a small positive epsilon (and log/raise on
non-positive values) before dividing so the normalization is consistent and
safe.
This pull request refactors and improves the stress and steady-state evaluation logic in the
weac.analysispackage. It introduces new result data classes and fixes plotting issues related to stress calculations and visualization.Refactoring and Data Model Improvements
MaximalStressResultandSteadyStateResultdata classes, replacing the oldSSERRResultfor clearer and more comprehensive result encapsulation. The new classes include normalized stress fields and system state at touchdown. ([[1]](https://github.com/2phi/weac/pull/34/files#diff-cd31ff029b0e336203154640e70d582b1554ca04f41c94048eaec4dee7daab6dL98-R129),[[2]](https://github.com/2phi/weac/pull/34/files#diff-cd31ff029b0e336203154640e70d582b1554ca04f41c94048eaec4dee7daab6dL110-R153),[[3]](https://github.com/2phi/weac/pull/34/files#diff-1b897e0700621752c15b1115da95c4b2a2d61aa57dde50d6b780bf490269b16cL11-R11),[[4]](https://github.com/2phi/weac/pull/34/files#diff-1b897e0700621752c15b1115da95c4b2a2d61aa57dde50d6b780bf490269b16cL21-R21))SSERRtoSteadyStatefor consistency and clarity, including renaming the evaluation method and result types. ([[1]](https://github.com/2phi/weac/pull/34/files#diff-cd31ff029b0e336203154640e70d582b1554ca04f41c94048eaec4dee7daab6dL644-R684),[[2]](https://github.com/2phi/weac/pull/34/files#diff-cd31ff029b0e336203154640e70d582b1554ca04f41c94048eaec4dee7daab6dL691-R734),[[3]](https://github.com/2phi/weac/pull/34/files#diff-1b897e0700621752c15b1115da95c4b2a2d61aa57dde50d6b780bf490269b16cL11-R11),[[4]](https://github.com/2phi/weac/pull/34/files#diff-1b897e0700621752c15b1115da95c4b2a2d61aa57dde50d6b780bf490269b16cL21-R21))Stress Calculation Enhancements
Sxxmethod inanalyzer.pyto support normalization of axial normal stress to tensile strength, improving numerical stability and result interpretability. ([[1]](https://github.com/2phi/weac/pull/34/files#diff-8468e03357e32957d46ec295646154a4a64e750fa249655a80c6620d9ad32440L228-R228),[[2]](https://github.com/2phi/weac/pull/34/files#diff-8468e03357e32957d46ec295646154a4a64e750fa249655a80c6620d9ad32440R242-R243),[[3]](https://github.com/2phi/weac/pull/34/files#diff-8468e03357e32957d46ec295646154a4a64e750fa249655a80c6620d9ad32440L261-R269),[[4]](https://github.com/2phi/weac/pull/34/files#diff-8468e03357e32957d46ec295646154a4a64e750fa249655a80c6620d9ad32440L277-R291))principal_stress_slabto correctly convert tensile strength units and normalize principal stresses. ([[1]](https://github.com/2phi/weac/pull/34/files#diff-8468e03357e32957d46ec295646154a4a64e750fa249655a80c6620d9ad32440R451-R452),[[2]](https://github.com/2phi/weac/pull/34/files#diff-8468e03357e32957d46ec295646154a4a64e750fa249655a80c6620d9ad32440L463-R478))_calculate_maximal_stresseshelper method to centralize maximal stress computation for steady-state evaluation. ([src/weac/analysis/criteria_evaluator.pyR1211-R1241](https://github.com/2phi/weac/pull/34/files#diff-cd31ff029b0e336203154640e70d582b1554ca04f41c94048eaec4dee7daab6dR1211-R1241))Plotting and Visualization Fixes
plot_deformedinplotter.pyto robustly handle NaN values in weak-layer coordinates, ensuring correct plotting and preventing errors when visualizing stress/displacement fields. ([[1]](https://github.com/2phi/weac/pull/34/files#diff-737cdaa926ddb7c8e3803d8568adddbe44745a23baca2d6bd39fc27e8a41a1dcL888-R934),[[2]](https://github.com/2phi/weac/pull/34/files#diff-737cdaa926ddb7c8e3803d8568adddbe44745a23baca2d6bd39fc27e8a41a1dcL971-R978))These changes collectively enhance the clarity of stress analysis and steady-state evaluation in the codebase.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.