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

tests on GPU - parent usage #739

Merged
merged 3 commits into from
Apr 22, 2024
Merged

tests on GPU - parent usage #739

merged 3 commits into from
Apr 22, 2024

Conversation

juliasloan25
Copy link
Member

@juliasloan25 juliasloan25 commented Apr 15, 2024 •
edited
Loading

Purpose

closes #748

Content

  • wrap parent calls in Array for comparisons
  • remove parent calls when updating field
  • update checkpointer_test.jl to use consistent comms_ctx
  • the following tests now pass on GPU:
    • bucket_tests.jl
    • climaatmos_tests.jl
    • prescr_seaice_tests.jl
    • slab_ocean_tests.jl
    • field_exchanger_tests.jl
    • interfacer_tests.jl
    • checkpointer_test.jl
  • I have read and checked the items on the review checklist.

@juliasloan25 juliasloan25 force-pushed the js/gpu_tests branch 2 times, most recently from d71c77f to 1431d30 Compare April 22, 2024 01:59
@juliasloan25 juliasloan25 mentioned this pull request Apr 22, 2024
Closed
@juliasloan25 juliasloan25 requested a review from LenkaNovak April 22, 2024 21:53
LenkaNovak
LenkaNovak approved these changes Apr 22, 2024
View reviewed changes
Copy link
Collaborator

@LenkaNovak LenkaNovak left a comment

Choose a reason for hiding this comment

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

LGTM. I think it closes #748 ? It could be a little clearer to use the same name for PRs and their leaf issues, so it's faster to check where this fits in. :)

test/flux_calculator_tests.jl
@@ -1,6 +1,7 @@
import Test: @test, @testset, @test_throws
import StaticArrays
import ClimaCore as CC
import ClimaParams
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

without it I get MethodError: no method matching ThermodynamicsParameters(::Type{Float32}), but I'm not sure why it's coming up in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh actually, it's because this test is called after other tests that load ClimaParams in runtests.jl. I noticed that we need it here too when I was running each test individually

@juliasloan25 juliasloan25 changed the title make tests GPU compatible tests on GPU - parent usage Apr 22, 2024
@juliasloan25 juliasloan25 enabled auto-merge April 22, 2024 23:40
@juliasloan25 juliasloan25 merged commit 69a3969 into main Apr 22, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LenkaNovak LenkaNovak LenkaNovak 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.

tests on GPU - parent usage
2 participants
@juliasloan25 @LenkaNovak