fix: include rotor inertia from spanwise blade mass in auto-RNA (#130)#131
Conversation
read_windio_rna lumped the blades as a bare point mass at the rotor apex, which captured the hub-to-tower-top translation but dropped the rotor's own diametral inertia from the blade mass spread along the span. On IEA-22 that dropped term is ~2.5x the tower-top rotary inertia the point-mass lump produced, so rigid-RNA tower FA/SS frequencies came out too high versus a rigid-rotor reference (thanks @TheMercer). Assemble the rotor as a rigid body instead: diag([I_polar, I_polar/2, I_polar/2]) about the hub, with I_polar = N_bl * integral (dm/ds) * r^2 ds and r = hub_radius + span*cos(cone) (hub diameter and cone angle used when present, else 0 / no cone). Only each blade's own sectional spin inertia is still excluded. Mass and CM are unchanged; the inertia (and coupled tower frequencies) change relative to 1.16.0. Updates the hand-computed assembly test and adds a rotor-inertia test (hub-radius and span-length sensitivity).
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5655fff7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| i_blades = np.diag( | ||
| [i_polar_rotor, 0.5 * i_polar_rotor, 0.5 * i_polar_rotor] | ||
| ) |
There was a problem hiding this comment.
With nonzero uptilt, this diagonal tensor is in the rotor/shaft frame, but i_tt later sums every i_body as a tower-top-frame tensor. Because the new blade inertia is not rotated by the shaft tilt before being added to bodies, tilted rotors lose the blade-inertia izx product and misallocate the large polar term between ixx and izz, so lumped_rna_cal=True returns wrong tower-top inertias and coupled frequencies for tilted WindIO turbines.
Useful? React with 👍 / 👎.
Codex flagged that the rotor (and hub) inertia tensors are in the shaft frame but were summed as tower-top tensors without rotating by the shaft tilt, so a tilted rotor lost the tensor izx product and misallocated the large polar term between ixx and izz. Rotate both shaft-frame tensors into the tower frame by the uptilt (shaft axis [sign_x*cos, 0, sin]) before the parallel-axis assembly; the nacelle tensor is already tower-frame and is left alone. Adds a tilt-rotation test (izx appears, trace preserved).
|
@codex review Good catch, fixed in bf63315. The hub and rotor inertia tensors are shaft-frame, so they are now rotated into the tower-top frame by the uptilt before the parallel-axis assembly (shaft axis |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf63315f65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # rotor's contribution to the tower-top rotary inertia. | ||
| i_polar_rotor = n_blades * i_polar_each | ||
| i_blades = np.diag( | ||
| [i_polar_rotor, 0.5 * i_polar_rotor, 0.5 * i_polar_rotor] |
There was a problem hiding this comment.
When hub.cone_angle is non-zero, _blade_span_mass_inertia projects the span with cos(cone) and this line assigns only I_polar/2 to the transverse axes. A coned blade also has a shaft-axis offset of roughly span * sin(cone); for a symmetric rotor that adds N_bl * ∫ dm * (span * sin(cone))² to both transverse moments about the hub. As written, WindIO rotors with precone silently under-report the tower-frame iyy/izz contribution from the blade mass distribution, so the auto-RNA inertia remains too small for exactly the cone-angle cases this new path is trying to handle.
Useful? React with 👍 / 👎.
The rigid-rotor diametral moment used I_polar/2, which is the flat-disc (perpendicular-axis) result and only holds for a planar rotor. A coned rotor sits its section masses off the rotor plane by span*sin(cone), so each blade adds N_bl * integral(dm * (span*sin(cone))**2) to both transverse moments about the hub. _blade_span_mass_inertia now returns the axial second moment alongside the radial one, and read_windio_rna folds it into the diametral term. The radial reach itself already used span*cos(cone), so the two are consistent. Behaviour is unchanged for an unconed rotor (sin(cone) = 0).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb5c7ba6b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| polar_second_moment = _trapezoid(mpl * radial * radial, s) | ||
| axial_second_moment = _trapezoid(mpl * axial * axial, s) |
There was a problem hiding this comment.
When the blade span is represented by the usual endpoint-to-endpoint linear reference-axis segment, applying the trapezoid rule after squaring radial/axial does not compute ∫ dm·r²; it integrates a chord over a quadratic. For the straight 100 kg/m, 0–50 m blade used in the new test this returns 6.25e6 kg·m² per blade, while the exact integral of the piecewise-linear geometry is 4.17e6 kg·m², inflating the added rotor inertia by 50% before multiplying by blade count. This directly biases the auto-RNA frequencies #130 is intended to fix; integrate the polynomial per segment or use a quadrature that is exact for the squared linear coordinate.
Useful? React with 👍 / 👎.
The second-moment integrals used the trapezoid rule, which is only exact for a linear integrand. mass*r**2 is cubic on each piecewise-linear segment (linear mass per length times a linear coordinate squared), so trapezoid chords over it and over-estimates on a coarse grid: a straight two-station 0-50 m blade came out 6.25e6 instead of the exact 4.17e6 per blade, a 50 % bias on the added rotor inertia. Replace it with _second_moment, a Simpson evaluation (endpoints plus the segment midpoint, which is the endpoint average for a linear coordinate). Simpson is exact through degree three, so it integrates the cubic without error. The mass integral keeps the trapezoid rule, which is exact for the linear mass-per-length integrand.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4be1d83137
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (m_nac, r_nac, i_nac), | ||
| (m_hub, apex, i_hub), | ||
| (m_blades, apex.copy(), np.zeros((3, 3))), | ||
| (m_blades, apex.copy(), i_blades), |
There was a problem hiding this comment.
When cone_angle is non-zero, the blade sections have a non-zero first moment along the shaft (span*sin(cone)), but this body is still located at apex while carrying an inertia tensor computed about that apex. The loop below applies m * (rsq * eye - outer(r, r)) as a CM-based parallel-axis shift, so for real overhang/uptilt cases it misses the cross terms from the coned rotor CM and also leaves cm_axial unchanged. Please either place the rotor body at its coned CM with inertia about that CM, or integrate/translate the distributed blade inertia directly to the tower top.
Useful? React with 👍 / 👎.
The parallel-axis theorem I_O = I_C + m(|r|^2 I - r outer r) only holds when the tensor is taken about the body centre of mass. The rotor tensor was formed about the hub apex, but a coned rotor's mass sits off the hub plane along the shaft by span*sin(cone), so its CM is offset from the apex. Using the apex both as the tensor reference and as the shift origin double-counts that offset and drops the cross terms for a rotor with real overhang and shaft tilt. Compute the axial first moment (exact per-segment Simpson), take the shaft-axis CM offset as first-moment / mass, reduce the axial second moment to that CM (a mass-weighted variance, always non-negative), and place the rotor body at apex + offset along the shaft axis. A flat rotor (sin(cone) = 0) leaves the offset and axial term zero, so the planar I_polar/2 split about the apex is unchanged. For a coned rotor this raises the tower-top vertical CM by the precone term real ElastoDyn also carries (~0.06 m on IEA-22); the deck adapter approximates the blades as a point mass at the apex and omits it, so the cross-check test now documents that ~1 % divergence.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d89bf1ca1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| span = coords[2] | ||
| radial = hub_r + span * cone_cos | ||
| axial = span * cone_sin |
There was a problem hiding this comment.
When a WindIO blade has non-zero reference_axis.x or reference_axis.y (prebend/sweep), this new inertia path still computes the rotor lever arm only from the z span. The mass integration above correctly follows the 3-D arc length, but the polar moment should include the section's perpendicular distance from the shaft axis; otherwise swept/prebent blades can have a materially wrong RNA inertia (e.g. a straight axis ending at y=30, z=50 is undercounted because the y² contribution is omitted).
Useful? React with 👍 / 👎.
The rotor polar moment took the section lever from the z span alone, so a swept or prebent blade was undercounted: the mass integration follows the full 3-D reference-axis arc, but the lever ignored the in-plane tangential sweep and the prebend projection. Resolve each section onto the rotor axes instead: in-plane radial r = hub_r + z*cos(cone) - x*sin(cone), tangential t = y, axial a = z*sin(cone) + x*cos(cone). The polar lever is then the full in-plane distance from the shaft axis r**2 + t**2, and the axial term carries the prebend projection too. A straight blade (x = y = 0) is unchanged.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90296794d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| i_axial_cm = max( | ||
| 0.0, n_blades * i_axial_each - m_blades * cm_axial_offset * cm_axial_offset | ||
| ) | ||
| i_diam_rotor = 0.5 * i_polar_rotor + i_axial_cm |
There was a problem hiding this comment.
When assembly.number_of_blades is 1 or 2, the new I_polar / 2 diametral split assumes an azimuthally axisymmetric rotor even though the code accepts those blade counts. For a flat two-bladed rotor, the span mass lies on one diameter, so one transverse inertia should carry essentially N·∫dm·r² while the orthogonal one is near zero; setting both to half corrupts the fore-aft/side-side tower-top inertias and frequencies for such WindIO files. Either reject unsupported blade counts or assemble the tensor from the actual blade azimuths.
Useful? React with 👍 / 👎.
The rotor tensor uses the axisymmetric transverse split I_diam = I_polar/2 + N*integral(dm*a**2), which is exact only for three or more evenly spaced blades, whose mass ring has isotropic in-plane inertia. A one- or two- bladed rotor lies on a single diameter, so its transverse inertia is azimuth-dependent and no single static tower-top lump represents it (one in-plane axis carries almost all of I_polar, the orthogonal one almost none). Rather than emit a corrupted fore-aft / side-side inertia, reject those blade counts with a message pointing at an explicit tip_mass. Zero blades (bare tower) and the usual three-bladed rotors are unaffected.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf23bbe35a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cone_ang = _finite_float(hub.get("cone_angle", 0.0), "hub cone_angle") | ||
| cone_cos = float(np.cos(cone_ang)) | ||
| cone_sin = float(np.sin(cone_ang)) |
There was a problem hiding this comment.
When a real WindIO file supplies cone_angle: 4 (the WindIO hub schema defines this field in degrees), this passes 4 directly to np.sin/np.cos, so the new blade inertia and coned-CM terms are evaluated at about 229° instead of 4°. For nonzero-precone turbines such as the IEA reference models, that gives a much smaller/negative radial lever and a huge axial offset, corrupting the auto-RNA inertia and coupled tower frequencies; convert this value to radians before computing cone_cos/cone_sin.
Useful? React with 👍 / 👎.
WindIO stores angles in radians (SI): the IEA references carry cone_angle 0.0698 (4 deg), uptilt 0.1047 (6 deg) and root twist 0.2094 (12 deg), and the auto-RNA reads them directly, which the IEA-22 deck cross-check confirms to <0.5 % on CM. A hand-authored file that leaves an angle in degrees (cone_angle: 4 meaning 4 deg) would otherwise be evaluated at 4 rad = 229 deg and silently corrupt the coned lever and axial offset. Guard cone_angle and uptilt against a physical +/-45 deg bound so that mistake fails loudly with a message naming the radians convention.
|
Thanks, addressed both. On the cone-angle units (P1), the premise is not right for the WindIO schema this reads. WindIO stores angles in radians (SI), not degrees. The IEA references bear that out directly: IEA-22 carries That said, the failure mode you describe is real for a hand-authored file that leaves an angle in degrees, so I have hardened it in 6690b5d: On the two-bladed rotor split (P2), that is already handled a few lines above, in bf23bbe: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6690b5d93f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| evaluate the trig at the wrong angle. | ||
| """ | ||
| ang = _finite_float(value, what) | ||
| if abs(ang) > max_abs: |
There was a problem hiding this comment.
The WindIO turbine schema defines both components.hub.cone_angle and drivetrain uptilt with units: deg, so conforming files commonly contain values like cone_angle: 4 or uptilt: 6. Passing those degree values through this radian bound makes read_windio_rna reject them as 229°/344°, so lumped_rna_cal=True fails on standard WindIO ontologies instead of assembling the RNA; these fields need to be converted from degrees before applying trig/bounds.
Useful? React with 👍 / 👎.
The WindIO v2 turbine schema annotates hub.cone_angle and drivetrain uptilt as degrees, but the IEA reference ontologies store radians (IEA-22 cone_angle 0.0698 = 4 deg, uptilt 0.1047 = 6 deg, matched to <0.5 % on CM against the ElastoDyn deck). A pure-radians reader rejects schema- conforming degree files; a pure-degrees reader mis-reads the reference files as ~0 precone. Physical precone and shaft tilt are small (well under ~20 deg), so the two encodings never overlap: interpret a stored magnitude at or below 0.5 as radians and a larger one as degrees, and reject a value non-physical under either (> 90). Both a 4 and a 0.0698 cone now assemble to the same 4 deg RNA.
|
You are right about the schema, and my earlier reply was wrong to call it a false positive. The WindIO v2 turbine schema does annotate The wrinkle is that the reference ontologies do not follow that annotation. The IEA-22 file this reads stores Fixed in 2900ff1 by disambiguating on magnitude. Physical rotor precone and shaft tilt are small, well under ~20 deg, so the two encodings never overlap: a stored magnitude at or below 0.5 is treated as radians (0.5 rad = 29 deg, already larger than any real precone or tilt) and a larger one as degrees, with anything above 90 rejected as non-physical either way. A On the two-bladed split, that remains handled a few lines above (bf23bbe rejects 1 or 2 blades before the tensor is built), so the flagged |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2900ff1215
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if mag <= 0.5: | ||
| return ang |
There was a problem hiding this comment.
For a WindIO v2 file that follows the documented degrees units but has a small non-zero cone_angle or uptilt (for example 0.25 degrees), this branch classifies the value as radians because every magnitude <= 0.5 is returned unchanged. That silently turns 0.25° into 14.3°, so read_windio_rna computes the rotor CM and inertia with a much larger cone/tilt instead of accepting a schema-conforming degrees value.
Useful? React with 👍 / 👎.
The magnitude heuristic cannot resolve a degrees file whose cone and tilt
are both sub-half-degree (a zero-tilt turbine with a fractional-degree
precone reads as radians). Rejecting small values is not an option either,
since a legitimate radians file routinely has a small precone with zero
tilt. Add an explicit escape hatch instead: read_windio_rna gains
angle_units ('auto' | 'rad' | 'deg'), forwarded from Tower.from_windio and
from_windio_with_monopile as rna_angle_units. 'auto' (default) keeps the
pair heuristic for the common case; 'rad' / 'deg' take the file at its word
for the rare ambiguous one. Unknown values are rejected.
|
Fixed in e3d6d81. You are right that the all-sub-degree case is genuinely ambiguous from magnitude alone, and rejecting small values is not viable because a normal radians file often has a small precone with zero tilt. So I have added the explicit escape hatch you suggested. On the two-bladed split, still handled at the blade-count guard above (bf23bbe rejects 1 or 2 blades before the tensor is built). |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3d6d81759
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if units == "auto": | ||
| biggest = max(abs(cone), abs(uptilt)) | ||
| units = "deg" if biggest > 0.5 else "rad" | ||
| if max(abs(cone), abs(uptilt)) > 90.0: |
There was a problem hiding this comment.
When callers pass rna_angle_units="rad", this guard still compares the raw radian value to 90.0 as if it were degrees. A cone or uptilt of 2.0 rad (~114°) is above the documented physical limit but passes because 2.0 < 90, so the subsequent sin/cos assembly returns a plausible RNA for a non-physical geometry instead of raising the intended error.
Useful? React with 👍 / 👎.
The 90 deg physical guard compared the raw cone / uptilt value to 90, which is only meaningful when the input is in degrees. Under angle_units=rad a 2.0 rad (~114 deg) angle passed the 2.0 < 90 check and assembled a plausible RNA for a non-physical geometry. Resolve to radians first, then bound the magnitude at pi/2, so the 90 deg limit holds whether the file was rad or deg.
|
Fixed in 6871e4f. The 90 deg guard now resolves the value to radians first and bounds the magnitude at pi/2, so the physical limit holds for either unit. A 2.0 rad (~114 deg) cone under angle_units='rad' is now rejected, with a test pinning it. The two-bladed split (P2) remains handled at the blade-count guard in bf23bbe, which raises for 1 or 2 blades before the tensor is assembled, so the I_polar/2 line the review points at is only reachable for three or more evenly spaced blades. Nothing further to change there. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6871e4f686
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| radial = hub_r + span * cone_cos - x_off * cone_sin | ||
| tangential = y_off | ||
| axial = span * cone_sin + x_off * cone_cos |
There was a problem hiding this comment.
When cone_angle and hub.diameter are both nonzero, these formulas leave the hub-radius offset entirely in the rotor plane and omit it from the axial offset. The WindIO/WISDEM geometry treats the projected rotor radius as (Rhub + blade z) * cos(precone) (see WISDEM's rotor-diameter definition), so the blade-root radius should be rotated by precone too: otherwise coned hubs overestimate the radial lever and miss a constant Rhub * sin(cone) shaft-axis CM offset, corrupting cm_axial and the rotor inertia for IEA-style files with hub diameter.
Useful? React with 👍 / 👎.
The polar lever left the hub radius unconed in the rotor plane and dropped it from the axial offset. WISDEM and ElastoDyn place the blade root at the hub radius along the coned pitch axis, so the whole (hub_r + span) length projects through the cone (WISDEM rotorR = Rtip*cos(precone) with Rtip = Rhub + L). Fold hub_r into the along-axis distance before projecting: radial = (hub_r + span)*cos(cone) - x*sin(cone), axial = (hub_r + span)* sin(cone) + x*cos(cone). For a coned rotor with a real hub radius this adds the Rhub*sin(cone) shaft-axis offset that was missing. Unconed rotors (cone = 0) are unchanged.
|
Good catch. Fixed in e38287b, and verified against the WISDEM convention (rotorR = Rtip*cos(precone) with Rtip = Rhub + L), which ElastoDyn shares. The blade root sits at the hub radius along the coned pitch axis, so the whole (hub_r + span) length now projects through the cone rather than leaving the hub radius flat in the rotor plane: radial = (hub_r + span)cos(cone) - xsin(cone), axial = (hub_r + span)sin(cone) + xcos(cone). For a coned rotor with a real hub radius this restores the Rhub*sin(cone) shaft-axis offset that was dropped; unconed rotors are unchanged. Added a test that a coned rotor with a hub radius carries the extra axial CM offset. The two-bladed split remains handled at the blade-count guard above (bf23bbe rejects 1 or 2 blades before the tensor is built). |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e38287b734
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| axis_dist = hub_r + span | ||
| radial = axis_dist * cone_cos - x_off * cone_sin | ||
| tangential = y_off | ||
| axial = axis_dist * cone_sin + x_off * cone_cos |
There was a problem hiding this comment.
When hub.diameter is present and cone_angle is nonzero, this treats the blade-root hub radius as part of the coned blade span: the radial lever becomes (hub_r + span) * cos(cone) and the axial offset gets an extra hub_r * sin(cone). That contradicts the rotor-inertia formula documented below in this function (r = hub_r + span*cos(cone)) and shifts every blade section by a spurious hub-radius axial offset, so coned rotors with nonzero hub diameter get biased tower-top CM and inertia. The coning should apply to the blade span/prebend relative to the root, while the hub radius remains the in-plane root offset.
Useful? React with 👍 / 👎.
A stale inline comment still described the polar lever as r = hub_r + span*cos(cone), from before the hub radius was coned. Update it to r = (hub_r + span)*cos(cone) so the comment matches the implementation and the surrounding docstrings.
|
@codex review |
|
This one reverses the previous review round, which asked for exactly the opposite on the same lines: "Project the hub radius through precone ... the WindIO/WISDEM geometry treats the projected rotor radius as (Rhub + blade z) * cos(precone)". That earlier request is the correct one, so I am keeping the coned hub radius. I verified it against the WISDEM source rather than either review: ccblade sets rotorR = Rtip * cos(precone) with Rtip = Rhub + blade_length, i.e. the whole (Rhub + span) projects through the cone, and the blade root at the hub radius therefore carries a real Rhub*sin(precone) shaft-axis offset. ElastoDyn shares that convention: PreCone is the angle of the blade pitch axis to the rotor plane, the axis passes through the apex, and the root sits at HubRad along it. Coning about the blade root instead, leaving the hub radius flat in the rotor plane, is the non-standard reading and would not match either code. The genuine issue underneath the flag was a stale inline comment that still read r = hub_r + span*cos(cone) from before the change, which is what looked contradictory. Fixed in 6e27d40 so the comment matches the code and the docstrings. The formula itself stays (hub_r + span)*cos(cone). The two-bladed split remains handled at the blade-count guard above (bf23bbe). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e27d40cf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| r_tilt = np.array( | ||
| [[sign_x * c_t, 0.0, -s_t], [0.0, 1.0, 0.0], [s_t, 0.0, sign_x * c_t]] |
There was a problem hiding this comment.
For upwind rotors (sign_x = -1), this transform maps the hub-frame z basis to [0, 0, -1] when uptilt == 0, but the WindIO hub rigid-body frame defines z as upward (see the hub elastic_properties description in the WindIO turbine schema: https://github.com/IEAWindSystems/windIO/blob/main/windIO/schemas/turbine/turbine_schema.yaml). When an ontology provides a full 6-vector hub inertia with non-zero Ixz or Iyz products, those products are transformed with the wrong sign in the tower-top tensor; diagonal/axisymmetric hubs hide the error. Use a hub-frame transform whose z column remains upward for upwind cases, separately from the outboard shaft direction used for blade CM placement.
Useful? React with 👍 / 👎.
| `I_polar = N_bl · ∫ (dm/ds) · r² ds` (`r = hub_radius + span·cos(cone)`), | ||
| using the hub diameter and cone angle when present. Only each blade's own | ||
| sectional spin inertia is still excluded. This changes the auto-RNA | ||
| inertia (and hence coupled tower frequencies) relative to 1.16.0; mass | ||
| and centre of mass are unchanged. This intentionally goes beyond the |
There was a problem hiding this comment.
This paragraph is inconsistent with the implementation for coned rotors: the code projects the hub radius through the cone as (hub_r + span) * cos(cone), and it places blade mass at rotor_cm = apex + cm_axial_offset * shaft_axis, so cm_axial changes whenever cone_angle is non-zero. Users comparing 1.16.0 results from the changelog would otherwise use the wrong inertia lever and wrongly assume the center of mass stayed fixed.
Useful? React with 👍 / 👎.
The note described the pre-review lever (r = hub_radius + span*cos(cone)) and claimed the centre of mass was unchanged. The rotor is now assembled about its true (coned) centre of mass with the coned pitch-axis lever (hub_radius + span)*cos(cone) including prebend and sweep, and cm_axial shifts by the precone term for a non-zero cone_angle. Update the note to match: the tensor is diag([I_polar, I_diam, I_diam]) about the rotor CM, and both inertia and (for coned rotors) the CM change relative to 1.16.0; only total mass is unchanged.
|
Thanks. Taking the four in turn. Changelog (P3): fixed in e0b1fce. The note now describes the coned lever (hub_radius + span)*cos(cone), the assembly about the rotor centre of mass, and that cm_axial shifts by the precone term for a non-zero cone_angle, with only total mass unchanged. Hub z-axis on upwind rotors (P2): this is a genuine subtlety and you are right that the WindIO hub frame defines z upward. It only affects a hub whose inertia tensor carries off-diagonal terms, because a diagonal hub tensor is invariant under the z-flip. Every reference hub I have is diagonal, including IEA-22 (hub system_inertia [1.88e6, 1.20e6, 1.20e6, 0, 0, 0]), and the nacelle tensor, which does carry off-diagonals, is in the tower-top frame and is not rotated. So there is no impact on any current case. Resolving the exact hub-frame handedness correctly (it interacts with the rotor spin-axis orientation, which for an upwind rotor genuinely needs the orientation flip) deserves a focused change verified against the WISDEM/WindIO frame code rather than being rushed into this PR. I will track it as a follow-up. Hub radius through precone (P2): settled in the previous comment with the WISDEM source (rotorR = Rtip*cos(precone), Rtip = Rhub + L). The coned formula stays. Two-bladed split (P2): handled at the blade-count guard in bf63315..bf23bbe, which rejects 1 or 2 blades before the tensor is assembled. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0b1fcee5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # tensor is already in the tower frame and is left alone. | ||
| c_t, s_t = float(np.cos(uptilt)), float(np.sin(uptilt)) | ||
| r_tilt = np.array( | ||
| [[sign_x * c_t, 0.0, -s_t], [0.0, 1.0, 0.0], [s_t, 0.0, sign_x * c_t]] |
There was a problem hiding this comment.
For rotor_orientation='Upwind', sign_x is -1, so this rotation maps the hub-frame z basis to [-sin(uptilt), 0, -cos(uptilt)] (at zero tilt, global -z). WindIO hub rigid-body inertia is defined in a hub-aligned frame whose z axis points upward, so any upwind hub system_inertia with nonzero product terms is transformed with the wrong signs before being added to the tower-top tensor. Preserve the upward z basis for the upwind case (flipping the lateral basis instead if needed for handedness) before rotating the hub tensor.
Useful? React with 👍 / 👎.
The WindIO hub-aligned frame is the tower-top frame tilted about the lateral y-axis (x along the shaft, z up including tilt), so the shaft-tilt rotation carries sign_x on the sin (off-diagonal) terms, not the cos. The previous matrix put sign_x on the cos terms, which for an upwind rotor flipped the signs of a hub tensor Ixy / Iyz. A diagonal hub (every current reference, incl. IEA-22) and the symmetric rotor tensor are invariant to the choice, so results are unchanged there, but a full off-diagonal hub inertia is now oriented correctly. The outboard shaft axis for the coned-CM offset is built explicitly, since it is the shaft line (opposite sense to the hub-frame x basis for an upwind rotor). Also assert the >= 3-blade precondition at the axisymmetric split, which the blade-count guard upstream already enforces.
|
Both fixed in bb3ddad. Hub z-axis (P2): you are right. Working it through against the WindIO schema, the hub-aligned frame is the tower-top frame tilted about the lateral y-axis (x along the shaft, z up including tilt), so the shaft-tilt rotation carries sign_x on the sin terms, not the cos. The previous matrix put sign_x on the cos terms, which for an upwind rotor flipped the sign of a hub tensor Ixy / Iyz. A diagonal hub and the symmetric rotor tensor are invariant to that choice (which is why the diagonal reference hubs, IEA-22 included, were unaffected), but a full off-diagonal hub inertia is now oriented correctly. The rotor spin axis lands on the same shaft line either way, so the rotor izx is unchanged. The outboard shaft axis for the coned-CM offset is now built explicitly, since it is the shaft line and points the opposite way to the hub-frame x basis for an upwind rotor. Added a test that at zero tilt an upwind hub's Iyz passes through unchanged. Two-bladed split (P2): added an explicit assert (n_blades == 0 or n_blades >= 3) right at the split so the axisymmetric precondition is local and machine-checkable. The blade-count guard upstream already rejects 1 or 2 blades, so the split is only ever reached for a zero or >= 3-blade rotor. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb3ddad7b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # so the whole (hub_r + span) length projects through the cone, matching | ||
| # WISDEM/ElastoDyn (rotorR = Rtip*cos(precone) = (Rhub + L)*cos(cone)) | ||
| # rather than leaving the hub radius unconed in the rotor plane. | ||
| axis_dist = hub_r + span |
There was a problem hiding this comment.
When a valid blade reference_axis.z is offset from zero, e.g. values: [20, 70] for a 50 m blade, the mass integration still uses the 50 m arc length but this line treats the absolute z coordinate as the radial span, placing every blade section 20 m too far from the hub. That inflates the new rotor polar/diametral inertia and the coned CM offset; the existing blade reader defines blade length as abs(z[-1] - z[0]), so the lever should be based on span - span[0] before adding hub_r.
Useful? React with 👍 / 👎.
reference_axis.z can be offset from zero (defined from a hub datum), but the rotor lever used the absolute z as the distance from the root, placing every section z[0] too far out and inflating the rotor inertia and coned CM offset. The mass integration already used the root-relative arc length, so only the lever was wrong. Subtract the root value: span = z - z[0]. A [20, 70] axis now gives the same rotor inertia and CM as [0, 50].
|
Good catch on the span, that one is a real bug. Fixed in 30fe99e. reference_axis.z can be offset from zero when it is defined from a hub datum, but the lever used the absolute z as the distance from the root, placing every section z[0] too far out and inflating the rotor inertia and coned CM offset. The mass integration already used the root-relative arc length, so only the lever was wrong. It now subtracts the root value, span = z - z[0], and a [20, 70] axis gives the same rotor inertia and CM as [0, 50]. Added a test for it. On the other two: Hub radius through precone: this reverses the review round before last, which asked for exactly this projection and cited WISDEM. I verified it against the WISDEM source (ccblade rotorR = Rtip*cos(precone) with Rtip = Rhub + blade_length), and ElastoDyn shares the convention, so the coned (hub_r + span)cos(cone) lever is correct and stays. The docstring it pointed at as contradictory was a stale comment, already fixed in 6e27d40; there is no longer any r = hub_r + spancos(cone) text in the function. Two-bladed split: rejected upstream at the blade-count guard, and there is now an explicit assert (n_blades == 0 or n_blades >= 3) at the split so the axisymmetric precondition is local and machine-checkable. It is unreachable for 1 or 2 blades. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Fixes #130. @TheMercer correctly spotted that the WindIO auto-RNA drops a large inertia term.
The bug
read_windio_rna(andlumped_rna_cal=True) lumped the blades as a bare point mass at the rotor apex. That captures the hub-to-tower-top translation (m_blades · apex²), but drops the rotor's own diametral inertia from the blade mass being spread along the span (Σ mᵢ · rᵢ²). For a rigid RNA lump that term is not small — on the IEA-22 blade it is:iyythe point-mass lump produced ≈ 1.42e8 kg·m²i.e. ~2.5× larger than what was there. So tower fore-aft / side-side frequencies from a rigid-RNA lump came out too high against a rigid-rotor reference (OrcaFlex), which is what he was seeing.
The fix
Assemble the rotor as a rigid body about the hub instead of a point mass:
using the hub diameter and cone angle when the ontology carries them (else 0 / no cone). Only each blade's own sectional spin inertia is still excluded. Mass and CM are unchanged; the inertia (and coupled tower frequencies) change relative to 1.16.0, so this ships as a Fixed entry.
This deliberately goes beyond the ElastoDyn deck path's point-mass lumping — the WindIO ontology's per-station blade mass is what makes the better term computable.
Tests
test_rna_rotor_inertia_from_span: the rotor term is present, grows with hub radius, and grows when the same blade mass is spread over a longer span.