1044 feature request material data timber#1045
Open
GerjanDorgelo wants to merge 21 commits into
Open
Conversation
…wood and hardwood according to EN 338:2016
…ing to EN 338:2016
…ation of structural timber
…or EN 338:2016 strength classes
…sed on timber class type
…r strength tables
2 tasks
|
Thank you so much for contributing to Blueprints! Now that you've created your pull request, please don't go away; take a look at the bottom of this page for the automated checks that should already be running. If they pass, great! If not, please click on 'Details' and see if you can fix the problem they've identified. A maintainer should be along shortly to review your pull request and help get it added! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements EN 338:2016 timber strength classification by adding three table modules (softwood bending, softwood tension, hardwood bending), a TimberMaterial dataclass that dispatches to the correct table, package/module docstrings, user documentation, and tests validating initialization, properties, error handling, and string representations. ChangesEN 338:2016 Timber Strength Classification & Material Interface
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TimberMaterial
participant Table
Caller->>TimberMaterial: instantiate(timber_class)
TimberMaterial->>TimberMaterial: __post_init__ -> validate & select Table
Caller->>TimberMaterial: read property (e.g., f_m_k)
TimberMaterial->>Table: forward property access
Table-->>TimberMaterial: return value
TimberMaterial-->>Caller: return value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@blueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_2.py`:
- Around line 60-65: The docstring and example use incorrect property names
(e_t_0_mean, e_t_0_k, e_t_90_mean) that don't exist; update them to the actual
API names e_m_0_mean, e_m_0_k, and e_m_90_mean in the class docstring and any
example access. Search for occurrences in table_2.py (class docstring and
example block) and replace the e_t_* identifiers and their descriptions with the
matching e_m_* symbols and correct wording (e.g., "Mean modulus of elasticity
parallel to grain (tension) -> e_m_0_mean", "5 percentile -> e_m_0_k",
"perpendicular -> e_m_90_mean") so the docs and examples match the implemented
attributes.
In `@docs/guides/concepts/codes/eurocode/en_338_2016/tables.md`:
- Around line 1-5: The heading has an unclosed bold marker and the intro
sentence is awkward; close the bold marker for the heading ("**EN 338 - April
2016") and rewrite the introductory sentence for clarity — e.g., "This table
lists formulas from EN 338 (April 2016) related to strength classes of
structural timber, indicates applicability (✗ or ✔), and provides any pertinent
remarks; the 'Object Name' column maps to corresponding Python entities in
Blueprints." Update the opening paragraph accordingly and ensure the symbols for
applicability are consistent.
In `@tests/materials/test_timber.py`:
- Around line 41-56: The test hardcoded expectations for compressive strength
f_c_0_k are incorrect; open the test_hardwood_class_d40_properties (and the
corresponding test around lines 58-73) and update the asserted numeric literal
for f_c_0_k to the correct value used by the TimberMaterial implementation
(verify by instantiating TimberMaterial(timber_class=HardwoodStrengthClass.D40)
and checking timber.f_c_0_k), i.e., replace the wrong hard-coded 30 (and the
analogous wrong value in the other test) with the correct EN 338 value as
returned by TimberMaterial so the assertions match the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d467aaf7-a332-47b3-a84b-d97615965e74
📒 Files selected for processing (13)
blueprints/codes/eurocode/en_338_2016/__init__.pyblueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/__init__.pyblueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_1.pyblueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_2.pyblueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_3.pyblueprints/materials/timber.pydocs/guides/concepts/codes/eurocode/en_338_2016/tables.mdtests/codes/eurocode/en_338_2016/__init__.pytests/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/__init__.pytests/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/test_table_1.pytests/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/test_table_2.pytests/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/test_table_3.pytests/materials/test_timber.py
…centile characteristic values
…lass T14 in tests
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1045 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 459 464 +5
Lines 14260 14547 +287
==========================================
+ Hits 14260 14547 +287 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_1.py (1)
264-273:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
__str__docstring mentions "hardwood properties" in a softwood class.Copy/paste leftover; same wording appears in
table_2.py(also softwood). Should say "timber" or "softwood".- Return a string representation of the hardwood properties. + Return a string representation of the softwood properties.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@blueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_1.py` around lines 264 - 273, The __str__ method's docstring incorrectly refers to "hardwood properties" in a softwood context; update the docstring in the __str__ method (in table_1.py) to correctly describe the output as "timber properties" or "softwood properties" (matching the class purpose), e.g., change the summary line and/or description to say "Return a string representation of the timber/softwood properties" while leaving the return format and f-string (self.timber_class, self.f_m_k, self.e_m_0_mean, self.rho_mean) unchanged.
🧹 Nitpick comments (2)
blueprints/materials/timber.py (2)
74-84: 💤 Low valueConsider caching the resolved table instance.
_tableis a@propertythat constructs a freshTableXStrengthClasses*instance on every access. Each public property (f_m_k,e_m_0_mean, thegetattr-based optional ones, etc.) triggers a new instantiation — i.e. another__post_init__validation pass — so reading several properties in a row from a singleTimberMaterialre-validates the class N times. Since the dataclass is frozen, you can resolve the table once in__post_init__and store it viaobject.__setattr__, e.g.:def __post_init__(self) -> None: """Validate that the timber class is a supported EN 338:2016 strength class.""" - if type(self.timber_class) not in _TIMBER_TABLE_BY_CLASS_TYPE: + table_cls = _TIMBER_TABLE_BY_CLASS_TYPE.get(type(self.timber_class)) + if table_cls is None: raise ValueError( f"Invalid timber class: {self.timber_class!r}. Must be one of " f"SoftwoodStrengthClassBending, SoftwoodStrengthClassTension or HardwoodStrengthClass." ) + object.__setattr__(self, "_table_instance", table_cls(self.timber_class))and have the
_tableproperty return that cached attribute. This also collapses the isinstance ladder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@blueprints/materials/timber.py` around lines 74 - 84, The _table property currently constructs a new Table1/2/3 instance on every access causing repeated __post_init__ validations; fix this by resolving the correct table once in the class __post_init__ (detect timber_class type there and instantiate the matching Table1StrengthClassesSoftwoodBending, Table2StrengthClassesSoftwoodTension, or Table3StrengthClassesHardwoodBending) and store it on the frozen dataclass using object.__setattr__ (e.g., self._resolved_table); then change the _table property to simply return that cached attribute and remove the repeated isinstance ladder so all downstream properties (f_m_k, e_m_0_mean, optional getattr-based properties) reuse the single cached table instance.
130-157: ⚡ Quick win
getattr(..., None)silently masks future typos in property names.Returning
Nonebased on a string lookup is brittle: if a table class ever renamese_t_0_mean(the existing one already happened during this PR), these properties will silently start returningNonefor what should be a valid case, and the test for that branch (assert ... is None) would still pass. Since the table type is uniquely determined bytype(self.timber_class), you can dispatch explicitly:- `@property` - def e_m_0_mean(self) -> MPA | None: - """...""" - return getattr(self._table, "e_m_0_mean", None) + `@property` + def e_m_0_mean(self) -> MPA | None: + """...""" + if isinstance(self._table, Table2StrengthClassesSoftwoodTension): + return None + return self._table.e_m_0_meanand analogously for the
e_t_*properties (returnNoneunless_tableis the tension table). That way a rename in a table class becomes a static/type error instead of a silentNone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@blueprints/materials/timber.py` around lines 130 - 157, Replace the fragile getattr(..., None) lookups by dispatching on the concrete table type so renames become type errors: for the bending properties (e_m_0_mean, e_m_0_k, e_m_90_mean) check that self._table is the bending table class (or use type(self.timber_class) / isinstance against the known BendingTable type) and return the attribute directly, otherwise return None; do the same for the tension properties (e_t_0_mean, e_t_0_k, e_t_90_mean) checking for the tension table class, so the code accesses self._table.e_* only when the table type matches and will fail fast on renames.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@blueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_1.py`:
- Around line 54-65: The class docstring lists stiffness properties (e_m_0_mean,
e_m_0_k, e_m_90_mean, g_mean) as GPA/kN/mm² while the property methods actually
multiply by GPA_TO_MPA and return MPA (N/mm²); update the class-level
"Properties" block to use MPA (N/mm²) for e_m_0_mean, e_m_0_k, e_m_90_mean and
g_mean (leave rho_k and rho_mean as KG_M3), and update the Examples section to
show the actual returned values (e.g., 11000.0) so the docstring matches the
`@property` behavior and per-property docstrings. Ensure terminology and units in
the class docstring match the existing property names and the GPA_TO_MPA
conversion already used.
In
`@blueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_2.py`:
- Around line 60-71: The docstring units and example in the class that defines
e_t_0_mean, e_t_0_k, e_t_90_mean, g_mean, rho_k, and rho_mean are wrong: the
properties currently return values multiplied by GPA_TO_MPA (MPa / N/mm²) but
the docstring lists GPA/kN/mm² and the Example shows 11.0; update the class
docstring to state units as MPa (N/mm²) for e_t_0_mean, e_t_0_k, e_t_90_mean and
g_mean (and keep rho_k/rho_mean as kg/m³), and change the Example value for T14
(e.g. table.e_t_0_mean → 11000.0) to match the implemented return values and the
test expectations (see e_t_0_mean, e_t_0_k, e_t_90_mean, g_mean, rho_k,
rho_mean).
In
`@blueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_3.py`:
- Around line 56-67: The docstring for the properties e_m_0_mean, e_m_0_k,
e_m_90_mean and g_mean is inconsistent with their implementation: the code
applies GPA_TO_MPA conversion and returns values in MPa (N/mm²), but the
docstring and Examples state GPA (kN/mm²) and show GPA values; update the
docstring unit/type and the Examples block to state MPa (N/mm²) and show the
converted values (e.g., D40 e_m_0_mean → 13000.0) so the documentation matches
the implemented properties (refer to e_m_0_mean, e_m_0_k, e_m_90_mean, g_mean
and the GPA_TO_MPA conversion used in this module).
---
Outside diff comments:
In
`@blueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_1.py`:
- Around line 264-273: The __str__ method's docstring incorrectly refers to
"hardwood properties" in a softwood context; update the docstring in the __str__
method (in table_1.py) to correctly describe the output as "timber properties"
or "softwood properties" (matching the class purpose), e.g., change the summary
line and/or description to say "Return a string representation of the
timber/softwood properties" while leaving the return format and f-string
(self.timber_class, self.f_m_k, self.e_m_0_mean, self.rho_mean) unchanged.
---
Nitpick comments:
In `@blueprints/materials/timber.py`:
- Around line 74-84: The _table property currently constructs a new Table1/2/3
instance on every access causing repeated __post_init__ validations; fix this by
resolving the correct table once in the class __post_init__ (detect timber_class
type there and instantiate the matching Table1StrengthClassesSoftwoodBending,
Table2StrengthClassesSoftwoodTension, or Table3StrengthClassesHardwoodBending)
and store it on the frozen dataclass using object.__setattr__ (e.g.,
self._resolved_table); then change the _table property to simply return that
cached attribute and remove the repeated isinstance ladder so all downstream
properties (f_m_k, e_m_0_mean, optional getattr-based properties) reuse the
single cached table instance.
- Around line 130-157: Replace the fragile getattr(..., None) lookups by
dispatching on the concrete table type so renames become type errors: for the
bending properties (e_m_0_mean, e_m_0_k, e_m_90_mean) check that self._table is
the bending table class (or use type(self.timber_class) / isinstance against the
known BendingTable type) and return the attribute directly, otherwise return
None; do the same for the tension properties (e_t_0_mean, e_t_0_k, e_t_90_mean)
checking for the tension table class, so the code accesses self._table.e_* only
when the table type matches and will fail fast on renames.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f9d448b-03a2-4f81-88b9-840bdcbc2e29
📒 Files selected for processing (7)
blueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_1.pyblueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_2.pyblueprints/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/table_3.pyblueprints/materials/timber.pydocs/guides/concepts/codes/eurocode/en_338_2016/tables.mdtests/codes/eurocode/en_338_2016/chapter_5_classification_of_structural_timber/test_table_2.pytests/materials/test_timber.py
✅ Files skipped from review due to trivial changes (1)
- docs/guides/concepts/codes/eurocode/en_338_2016/tables.md
… hardwood classes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds Timber material data, in exactly the way steel has been done.
Fixes #1044
Type of change
Checklist:
Summary by CodeRabbit
New Features
Documentation
Tests