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.

Sign up for GitHub

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

Jump to bottom

Fix fft_poisson_solver constructor #3890

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix fft_poisson_solver constructor #3890

wants to merge 2 commits into from
+15 −1

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Oct 31, 2024 •
edited
Loading

Closes #3889

@glwagner
Copy link
Member Author

glwagner commented Oct 31, 2024 •
edited
Loading

@liuchihl check out the similarity between the simple test that I added and the MWE on #3889. I just copy-pasted the MWE code into the test (and made a few generalizations). It should be ready to merge, and it all only took a few minutes.

@glwagner glwagner requested a review from navidcy October 31, 2024 22:36
navidcy
navidcy approved these changes Oct 31, 2024
View reviewed changes
Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

lgtm

@navidcy
Copy link
Collaborator

navidcy commented Oct 31, 2024

Did we break the distributed tests?
https://buildkite.com/clima/oceananigans-distributed/builds/4176#0192e4ca-db93-47a2-a550-47e8a36b6bc2/232-1228

@glwagner
Copy link
Member Author

https://buildkite.com/clima/oceananigans-distributed/builds/4176#0192e4ca-db93-47a2-a550-47e8a36b6bc2/232-1228

We fixed a bug whereby they were not running. However, they did pass. But it does seem like there is instability, maybe because we don't always successfuly get CUDA-aware MPI

@glwagner
Copy link
Member Author

Note to self that cpu_poisson_solver_tests_2 is relatively cheap so we should add the CG Poisson solver tests there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@navidcy navidcy navidcy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error in ConjugateGradientPoissonSolver for nonuniform grids
2 participants
@glwagner @navidcy