Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert code blocks documenting ImmersedBoundaryCondition to doctests #3705

Open
glwagner opened this issue Aug 13, 2024 · 10 comments
Open

Convert code blocks documenting ImmersedBoundaryCondition to doctests #3705

glwagner opened this issue Aug 13, 2024 · 10 comments
Labels
testing 🧪 Tests get priority in case of emergency evacuation

Comments

@glwagner
Copy link
Member

glwagner commented Aug 13, 2024

@glwagner, could you open an issue pointing to this PR and saying that some doctests were removed and we need to attend to them with proper filtering etc?

Originally posted by @navidcy in #3692 (review)

@glwagner
Copy link
Member Author

Some doctests for ImmersedBoundaryCondition were converted to ordinary code blocks in #3692. It'd be nice to convert these back to doctests, if we can figure out how to filter the warnings.

Another solution is to eliminate the warnings themselves. ImmersedBoundaryCondition may be ready for warnings to be removed and the example could use HydrostaticFreeSurfaceModel in which case there'd be no warning.

@navidcy navidcy added the testing 🧪 Tests get priority in case of emergency evacuation label Aug 14, 2024
@tomchor
Copy link
Collaborator

tomchor commented Aug 14, 2024

Some doctests for ImmersedBoundaryCondition were converted to ordinary code blocks in #3692. It'd be nice to convert these back to doctests, if we can figure out how to filter the warnings.

If I understand correctly I remember having to do this in #2990, specifically this file.

The header has to start with a regex expression like:

```jldoctest; filter = r"└ @ Oceananigans.BuoyancyModels.*"

@glwagner
Copy link
Member Author

Hmm right... that filter stopped working. But now I am realizing there is something peculiar. The filter matches with the last line of the warning --- which acts to filter out the entire warning? Do you understand why that is?

That might be the key. The confusing thing about the issue here is that there are multiple warnings. But if we only need to filter the last line of the final warning, perhaps that's what we werent' doing...

@tomchor
Copy link
Collaborator

tomchor commented Aug 14, 2024

Hmm right... that filter stopped working.

I think it's still working. I found an instance of it still being used here:

```jldoctest; filter = r".*@ Oceananigans.MultiRegion.*"
julia> using Oceananigans
julia> grid = RectilinearGrid(size=(12, 12), extent=(1, 1), topology=(Bounded, Bounded, Flat))
12×12×1 RectilinearGrid{Float64, Bounded, Bounded, Flat} on CPU with 3×3×0 halo
├── Bounded x ∈ [0.0, 1.0] regularly spaced with Δx=0.0833333
├── Bounded y ∈ [0.0, 1.0] regularly spaced with Δy=0.0833333
└── Flat z
julia> multi_region_grid = MultiRegionGrid(grid, partition = XPartition(4))
┌ Warning: MultiRegion functionalities are experimental: help the development by reporting bugs or non-implemented features!
└ @ Oceananigans.MultiRegion ~/Research/OC11.jl/src/MultiRegion/multi_region_grid.jl:108
MultiRegionGrid{Float64, Bounded, Bounded, Flat} partitioned on CPU():

The filter matches with the last line of the warning --- which acts to filter out the entire warning? Do you understand why that is?

It matches the last line of the warning because that's the only line that changes from run to run. In particular the part tartarus-16 could be different it it ends up running on a different node:

┌ Warning: The meaning of `gravity_unit_vector` changed in version 0.80.0.
│ In versions 0.79 and earlier, `gravity_unit_vector` indicated the direction _opposite_ to gravity.
│ In versions 0.80.0 and later, `gravity_unit_vector` indicates the direction of gravitational acceleration.
└ @ Oceananigans.BuoyancyModels ~/builds/tartarus-16/clima/oceananigans/src/BuoyancyModels/buoyancy.jl:48

The other lines of the warning are always the same, so they don't need filtering.

@glwagner
Copy link
Member Author

I think it's still working. I found an instance of it still being used here:

I meant for the specific instance that this issue refers to.

It matches the last line of the warning because that's the only line that changes from run to run. In particular the part tartarus-16 could be different it it ends up running on a different node:

My question is: why does matching the last line of the warning act to remove the whole warning? I am not asking about the intention of the filter. The intention is clear.

@glwagner
Copy link
Member Author

Oof, ok I understand. I thought the filtering would allow us to remove the warnings from the docstring. But it doesn't. Instead we have to include the warning in the docstring (including a random path to someone's Oceananigans version, eg Research/OC11.jl:

help?> MultiRegionGrid
search: MultiRegionGrid multi_region_grid MultiRegionField MultiRegionObject

  MultiRegionGrid(global_grid; partition = XPartition(2),
                               devices = nothing,
                               validate = true)

  Split a global_grid into different regions handled by devices.

  Positional Arguments
  ≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡

    •  global_grid: the grid to be divided into regions.

  Keyword Arguments
  ≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡

    •  partition: the partitioning required. The implemented partitioning are XPartition (division along the x direction) and
       YPartition (division along the y direction).

    •  devices: the devices to allocate memory on. If nothing is provided (default) then memorey is allocated on the the CPU.
       For GPU computation it is possible to specify the total number of GPUs or the specific GPUs to allocate memory on. The
       number of devices does not need to match the number of regions.

    •  validate :: Boolean: Whether to validate devices; defautl: true.

  Example
  ≡≡≡≡≡≡≡

  julia> using Oceananigans

  julia> grid = RectilinearGrid(size=(12, 12), extent=(1, 1), topology=(Bounded, Bounded, Flat))
  12×12×1 RectilinearGrid{Float64, Bounded, Bounded, Flat} on CPU with 3×3×0 halo
  ├── Bounded  x ∈ [0.0, 1.0] regularly spaced with Δx=0.0833333
  ├── Bounded  y ∈ [0.0, 1.0] regularly spaced with Δy=0.0833333
  └── Flat z

  julia> multi_region_grid = MultiRegionGrid(grid, partition = XPartition(4))
  ┌ Warning: MultiRegion functionalities are experimental: help the development by reporting bugs or non-implemented features!
  └ @ Oceananigans.MultiRegion ~/Research/OC11.jl/src/MultiRegion/multi_region_grid.jl:108
  MultiRegionGrid{Float64, Bounded, Bounded, Flat} partitioned on CPU():
  ├── grids: 3×12×1 RectilinearGrid{Float64, RightConnected, Bounded, Flat} on CPU with 3×3×0 halo
  ├── partitioning: Equal partitioning in X with (4 regions)
  ├── connectivity: MultiRegionObject{Tuple{@NamedTuple{west::Nothing, east::Oceananigans.MultiRegion.RegionalConnectivity{Oceananigans.MultiRegion.East, Oceananigans.MultiRegion.West}, north::Nothing, south::Nothing}, @NamedTuple{west::Oceananigans.MultiRegion.RegionalConnectivity{Oceananigans.MultiRegion.West, Oceananigans.MultiRegion.East}, east::Oceananigans.MultiRegion.RegionalConnectivity{Oceananigans.MultiRegion.East, Oceananigans.MultiRegion.West}, north::Nothing, south::Nothing}, @NamedTuple{west::Oceananigans.MultiRegion.RegionalConnectivity{Oceananigans.MultiRegion.West, Oceananigans.MultiRegion.East}, east::Oceananigans.MultiRegion.RegionalConnectivity{Oceananigans.MultiRegion.East, Oceananigans.MultiRegion.West}, north::Nothing, south::Nothing}, @NamedTuple{west::Oceananigans.MultiRegion.RegionalConnectivity{Oceananigans.MultiRegion.West, Oceananigans.MultiRegion.East}, east::Nothing, north::Nothing, south::Nothing}}, NTuple{4, CPU}}
  └── devices: (CPU(), CPU(), CPU(), CPU())

That's not as nice as I was hoping.

@glwagner
Copy link
Member Author

I think warnings in docstrings are not nice. Perhaps the right solution here is to remove the warning for ImmersedBoundaryCondition and also to change the doctest to use HydrostaticFreeSurfaceModel.

@glwagner
Copy link
Member Author

Strikes me that we could also change the log level so that warnings are not emitted.

@tomchor
Copy link
Collaborator

tomchor commented Aug 14, 2024

Personally I don't see a problem with warnings in the docstrings if that's how the code behaves.

Strikes me that we could also change the log level so that warnings are not emitted.

I'm not sure what the expected result of this is. But if this will create a situation where a given docstring won't have warnings, while users copy-pasting the contents from that same docstring may get a warning, then I think it'll be confusing and we should probably avoid that solution. If not, then that sounds like a great solution :)

@glwagner
Copy link
Member Author

We wrote a more verbose version of the warning already in the docs. When 4 warnings print on top of each other, it makes the docs harder to read and therefore less effective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing 🧪 Tests get priority in case of emergency evacuation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants