Add docstring to MutableVerticalDiscretization#5282
Conversation
glwagner
left a comment
There was a problem hiding this comment.
suggest to use DocStringExtensions. to deduce how they work, use a google search!
|
Have a look in GeophysicalFlows. We use DocStrinExtensions for struct docstrings there. |
|
oh... sorry, I might have pushed over you! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5282 +/- ##
==========================================
+ Coverage 68.79% 73.51% +4.72%
==========================================
Files 395 395
Lines 21465 22192 +727
==========================================
+ Hits 14767 16315 +1548
+ Misses 6698 5877 -821
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Added a "Fields" title before they are listed (had a look at the PR preview docs and I think this was missing). Also just trying |
|
With help?> MutableVerticalDiscretization
search: MutableVerticalDiscretization ExponentialDiscretization VerticallyImplicitTimeDiscretization ReferenceToStretchedDiscretization
struct MutableVerticalDiscretization{C, D, E, F, H, CC, FC, CF, FF} <: AbstractVerticalCoordinate
Represent a mutable vertical coordinate that can evolve in time.
Fields
≡≡≡≡≡≡
• cᵃᵃᶠ::Any: Face-centered reference coordinate
• cᵃᵃᶜ::Any: Cell-centered reference coordinate
• Δᵃᵃᶠ::Any: Face-centered grid spacing
• Δᵃᵃᶜ::Any: Cell-centered grid spacing
• ηⁿ::Any: Surface elevation at the current time step
• σᶜᶜⁿ::Any: (Center, Center) scaling factor at the current time step
• σᶠᶜⁿ::Any: (Face, Center) scaling at the current time step
• σᶜᶠⁿ::Any: (Center, Face) scaling at the current time step
• σᶠᶠⁿ::Any: (Face, Face) scaling factor at the current time step
• σᶜᶜ⁻::Any: (Center, Center) scaling factor at the previous time step
• ∂t_σ::Any: Time derivative of the cell-centered scaling factorwhile with help?> MutableVerticalDiscretization
search: MutableVerticalDiscretization ExponentialDiscretization VerticallyImplicitTimeDiscretization ReferenceToStretchedDiscretization
struct MutableVerticalDiscretization{C, D, E, F, H, CC, FC, CF, FF} <: AbstractVerticalCoordinate
Represent a mutable vertical coordinate that can evolve in time.
Fields
≡≡≡≡≡≡
• cᵃᵃᶠ: Face-centered reference coordinate
• cᵃᵃᶜ: Cell-centered reference coordinate
• Δᵃᵃᶠ: Face-centered grid spacing
• Δᵃᵃᶜ: Cell-centered grid spacing
• ηⁿ: Surface elevation at the current time step
• σᶜᶜⁿ: (Center, Center) scaling factor at the current time step
• σᶠᶜⁿ: (Face, Center) scaling at the current time step
• σᶜᶠⁿ: (Center, Face) scaling at the current time step
• σᶠᶠⁿ: (Face, Face) scaling factor at the current time step
• σᶜᶜ⁻: (Center, Center) scaling factor at the previous time step
• ∂t_σ: Time derivative of the cell-centered scaling factor
Why do we want all those |
|
Oh dang, I was hoping it would show the type parameter names that match the struct definition, like so (edited by hand): help?> MutableVerticalDiscretization
search: MutableVerticalDiscretization ExponentialDiscretization VerticallyImplicitTimeDiscretization ReferenceToStretchedDiscretization
struct MutableVerticalDiscretization{C, D, E, F, H, CC, FC, CF, FF} <: AbstractVerticalCoordinate
Represent a mutable vertical coordinate that can evolve in time.
Fields
≡≡≡≡≡≡
• cᵃᵃᶠ::C: Face-centered reference coordinate
• cᵃᵃᶜ::D: Cell-centered reference coordinate
• Δᵃᵃᶠ::E: Face-centered grid spacing
• Δᵃᵃᶜ::F: Cell-centered grid spacing
• ηⁿ::H: Surface elevation at the current time step
• σᶜᶜⁿ::CC: (Center, Center) scaling factor at the current time step
• σᶠᶜⁿ::FC: (Face, Center) scaling at the current time step
• σᶜᶠⁿ::CF: (Center, Face) scaling at the current time step
• σᶠᶠⁿ::FF: (Face, Face) scaling factor at the current time step
• σᶜᶜ⁻::CC: (Center, Center) scaling factor at the previous time step
• ∂t_σ::CC: Time derivative of the cell-centered scaling factorI also didn't think I could simply use EDIT: I don't think there's a clean way to do that so I'm just reverting back to using |
But even if it showed |
I agree showing Anyway I agree that not showing the type is better now, as I realize that showing |
|
Doc preview looks great. I’ll merge |
* Support boolean comparison operations on fields and infer eltype from operations Register comparison operators (>, <, >=, <=) as binary operations on fields, and use Base.promote_op to infer the result type of all AbstractOperation constructors instead of hardcoding eltype(grid). Field(op) now allocates data matching the operation's eltype, so boolean operations produce Bool arrays. Closes #5307 Co-Authored-By: Claude Opus 4.6 <[email protected]> * Fix doctest and operator precedence in comparison tests - Suppress Set output in @binary doctest (non-deterministic ordering) - Wrap comparison `isa` tests in parentheses to fix precedence (`u > v isa T` parses as `u > (v isa T)`, not `(u > v) isa T`) Co-Authored-By: Claude Opus 4.6 <[email protected]> * Fall back to eltype(grid) when promote_op fails for KernelFunctionOperation Base.promote_op can return Union{} or non-concrete types (like Any) for complex kernel functions (e.g. ForcingKernelFunction, buoyancy_perturbation). This caused MethodErrors when trying to allocate data arrays. Fall back to eltype(grid) when promote_op cannot infer a concrete return type. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Bump ambiguity threshold from 345 to 355 for comparison operators Adding >, <, >=, <= as binary operations on fields introduces 10 new method ambiguities with Base's definitions. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Refactor operation constructors: accept T as parameter, infer at higher level - All operation inner constructors now accept `::Type{T}` as an optional final argument with a sensible default, making them clean passthroughs - BinaryOperation, UnaryOperation, MultiaryOperation: default T via `Base.promote_op` (reliable for simple eltypes) - KernelFunctionOperation inner constructor takes `(kernel_function, grid, arguments::Tuple, T=eltype(grid))` — no argument splatting, usable as internal interface. Outer convenience constructor splats + infers T. - adapt_structure, on_architecture, and multi-region getregion pass `eltype(op)` explicitly, avoiding redundant re-inference Co-Authored-By: Claude Opus 4.6 <[email protected]> * Remove promote_op from KernelFunctionOperation constructor Kernel functions are too complex for Base.promote_op to reliably infer return types. For example, SeawaterPolynomials.ρ with a Float32 grid but Float64 EOS coefficients causes promote_op to return Float64, changing the KFO eltype from Float32 to Float64 and breaking Float32 seawater density tests. Revert to always using eltype(grid) for KFO, as on main. The promote_op eltype inference is still used for Binary, Unary, and Multiary operations where it works reliably. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Remove promote_op from KFO outer constructor, use eltype(grid) default promote_op inferred Float64 for TEOS10 density on Float32 grids (because EOS polynomial coefficients are Float64), causing precision mismatches in seawater density tests. The inner constructor already defaults to eltype(grid); users can override T explicitly via the inner constructor when needed. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Retrigger CI --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
This PR just adds a docstring to
MutableVerticalDiscretizationas I believe this was missing and I wanted to know what was in there. I let AI fill it up, and it makes sense to me, but I am not 100% sure it's correct, so this should be double-checked by someone else before being merged. No change to the code otherwise though!