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

Solves bug when calling ConjugateGradientPoissonSolver without specifying preconditioner #3846

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tomchor
Copy link
Collaborator

@tomchor tomchor commented Oct 10, 2024

@@ -1,4 +1,5 @@
using Oceananigans.Operators: divᶜᶜᶜ, ∇²ᶜᶜᶜ
using Oceananigans.Grids: ImmersedBoundaryGrid
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think ImmersedBoundaryGrid is defined by ImmersedBoundaries, which isn't imported until after Solvers:

include("Solvers/Solvers.jl")
include("OutputReaders/OutputReaders.jl")
include("DistributedComputations/DistributedComputations.jl")
# TODO: move here
#include("ImmersedBoundaries/ImmersedBoundaries.jl")
#include("MultiRegion/MultiRegion.jl")
# Physics, time-stepping, and models
include("Coriolis/Coriolis.jl")
include("BuoyancyModels/BuoyancyModels.jl")
include("StokesDrifts.jl")
include("TurbulenceClosures/TurbulenceClosures.jl")
include("Forcings/Forcings.jl")
include("Biogeochemistry.jl")
# TODO: move above
include("ImmersedBoundaries/ImmersedBoundaries.jl")

A bit of refactoring is needed to import ImmersedBoundaries before Solvers. So I think if we want to resolve this without all that refactoring we should define the constructor that depends on ImmersedBoundaryGrid separately (either in ImmersedBoundaries or Model.NonhydrostaticModels)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I think you're right!

@glwagner
Copy link
Member

I can fix this if you want

@tomchor
Copy link
Collaborator Author

tomchor commented Oct 11, 2024

I can fix this if you want

Thanks, yeah feel free to jump in!

@ali-ramadhan
Copy link
Member

This might be a duplicate of PR #3830 but looks like both will be superseded by PR #3847 which is doing the refactoring necessary for this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants