Skip to content

Add docstring to MutableVerticalDiscretization#5282

Merged
navidcy merged 13 commits intomainfrom
bp/mutableverticaldiscretization-docstring
Feb 13, 2026
Merged

Add docstring to MutableVerticalDiscretization#5282
navidcy merged 13 commits intomainfrom
bp/mutableverticaldiscretization-docstring

Conversation

@briochemc
Copy link
Copy Markdown
Collaborator

This PR just adds a docstring to MutableVerticalDiscretization as 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!

Copy link
Copy Markdown
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest to use DocStringExtensions. to deduce how they work, use a google search!

@navidcy
Copy link
Copy Markdown
Member

navidcy commented Feb 12, 2026

Have a look in GeophysicalFlows. We use DocStrinExtensions for struct docstrings there.

@navidcy
Copy link
Copy Markdown
Member

navidcy commented Feb 12, 2026

oh... sorry, I might have pushed over you!
I just changed TYPEDFIELDS to FIELDS since all types were Any.

@navidcy navidcy self-requested a review February 12, 2026 09:25
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.51%. Comparing base (5842259) to head (40020d8).

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     
Flag Coverage Δ
buildkite 68.79% <100.00%> (ø)
julia 68.79% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@briochemc
Copy link
Copy Markdown
Collaborator Author

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 TYPEDFIELDS again, hoping it helps link the field types with the type parameters.

@navidcy
Copy link
Copy Markdown
Member

navidcy commented Feb 12, 2026

With TYPEDFIELDS we get:

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 factor

while with FIELDS we get:

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 ::Any with TYPEDFIELDS? They don't give any useful information, right?

@briochemc
Copy link
Copy Markdown
Collaborator Author

briochemc commented Feb 12, 2026

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 factor

I also didn't think I could simply use help?> and thought I had to build the docs instead to see these, hence the commits... 🤦 Sorry about that! I'll try to see if I can get FIELDS to show the matching type parameter names.


EDIT: I don't think there's a clean way to do that so I'm just reverting back to using FIELDS.

@navidcy
Copy link
Copy Markdown
Member

navidcy commented Feb 12, 2026

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 factor

But even if it showed CC or FF those are just parameter types like dummy variables; they mean nothing really.. right? That's why it shows ::Any, right?

@briochemc
Copy link
Copy Markdown
Collaborator Author

But even if it showed CC or FF those are just parameter types; they mean nothing. That's why it shows ::Any, right?

I agree showing Any is not useful, but showing the type parameter name, like CC, might be? It's a tiny bit of extra information after all, and that can be helpful sometimes? For example, if I know what σᶜᶜⁿ is but I am not sure about ∂t_σ, knowing that they share the same type CC can help figure out what ∂t_σ is?

Anyway I agree that not showing the type is better now, as I realize that showing CC would cause another problem: The user would not know if CC is just a name or if it's a defined type.

@navidcy
Copy link
Copy Markdown
Member

navidcy commented Feb 13, 2026

Doc preview looks great. I’ll merge

@navidcy navidcy merged commit a63cb5c into main Feb 13, 2026
68 of 76 checks passed
@navidcy navidcy deleted the bp/mutableverticaldiscretization-docstring branch February 13, 2026 03:14
glwagner referenced this pull request Feb 20, 2026
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants