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

Update Oceananigans/KernelAbstractions versions #110

Merged
merged 23 commits into from
Jun 24, 2023
Merged

Conversation

jagoosw
Copy link
Collaborator

@jagoosw jagoosw commented Jun 23, 2023

No description provided.

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jun 23, 2023

With the new kernel launching, do I need to do something to get the threads to sync now that there's no wait!(event) going on? I can't see anything that looks like it tries to synchronise things in the Oceananigans repo

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jun 23, 2023

This will be a breaking change due because of removing the interface to specify advection schemes for the drift velocity (since Oceananigans no longer supports in since CliMA/Oceananigans.jl#3027)

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jun 23, 2023

Also, revisiting how I've implemented sediment integration because of some breaking changes from Oceananigans has reaffirmed to me that I still don't like how I've implemented it because it has to repeat so much from Oceananigans and uses non-public functions that break often

@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Patch coverage: 60.60% and project coverage change: +8.12 🎉

Comparison is base (1be82ca) 53.27% compared to head (2207525) 61.40%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   53.27%   61.40%   +8.12%     
==========================================
  Files          26       26              
  Lines        1023      969      -54     
==========================================
+ Hits          545      595      +50     
+ Misses        478      374     -104     
Impacted Files Coverage Δ
src/Light/morel.jl 0.00% <0.00%> (ø)
src/Utils/timestep.jl 81.81% <0.00%> (ø)
src/Utils/negative_tracers.jl 16.66% <20.00%> (+16.66%) ⬆️
src/Models/AdvectedPopulations/LOBSTER/LOBSTER.jl 81.25% <50.00%> (-3.37%) ⬇️
src/Models/AdvectedPopulations/NPZD.jl 90.41% <50.00%> (+90.41%) ⬆️
src/Particles/Particles.jl 23.52% <50.00%> (ø)
src/Models/Individuals/SLatissima.jl 83.87% <83.33%> (-0.31%) ⬇️
src/Boundaries/Sediments/Sediments.jl 61.53% <100.00%> (-9.05%) ⬇️
src/Boundaries/Sediments/coupled_timesteppers.jl 80.95% <100.00%> (-3.67%) ⬇️
src/Light/2band.jl 86.11% <100.00%> (-0.38%) ⬇️
... and 1 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@navidcy
Copy link
Collaborator

navidcy commented Jun 23, 2023

Is it a good idea to test some of the examples on a GPU since the tests don’t run on GPU as far as I recall?

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jun 23, 2023

Is it a good idea to test some of the examples on a GPU since the tests don’t run on GPU as far as I recall?

That's a good idea, I'll try running the examples on our GPU node.

I do intend to setup GPU testing like Oceananigans has at some point but it seems relatively challenging?

@navidcy
Copy link
Collaborator

navidcy commented Jun 23, 2023

I do intend to setup GPU testing like Oceananigans has at some point but it seems relatively challenging?

I wan't part of setting that up -- not sure how to set the tests to run on your own machine.

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jun 23, 2023

The examples column.jl, kelp.jl, and eady.jl (in 16s!) all seem to work just by changing the architecture to GPU and I think between them they should cover everything except the NPZD model. I have also realised that the NPZD example just doesn't work, so I will fix.

@navidcy
Copy link
Collaborator

navidcy commented Jun 23, 2023

Is there a reason the NPZD example is not included in the list of examples that are literated in the docs?

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jun 23, 2023

I was just wondering this, I've just had a go at running it and I don't think I ever really finished it. Perhaps we should just get rid of it?

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jun 23, 2023

This is a video of the first 10 days and I don't think it looks like I ever set it up to be physically interesting, or tuned the initial conditions to get the BGC to show anything.

npzd.mp4

I guess I was attempting to replicate something like this which I guess is doable but would need to find initial NPZ conditions that lead to some surface growth. Probably why its currently set to run for 2years so it could converge.

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jun 23, 2023

Perhaps we could leave it out for now and sort it out elsewhere? Just I was going to submit the JOSS paper today and you have to put a version, so I wanted to merge this first because its got quite a lot of changes.

@navidcy
Copy link
Collaborator

navidcy commented Jun 23, 2023

Yes, nuke the NPZD example. It'll be there in the git history and we can add it later!

OK! I'm happy to approve this PR since you confirm that you ran the examples on GPU and all is good.

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jun 23, 2023

Yes, nuke the NPZD example. It'll be there in the git history and we can add it later!

OK! I'm happy to approve this PR since you confirm that you ran the examples on GPU and all is good.

Okay great, thank you! I've also just realised that the quick start example also uses the NPZD model so it is getting run in the docs, so this example probably isn't very instructional anyway and is confusing

@navidcy navidcy self-requested a review June 23, 2023 21:21
@navidcy
Copy link
Collaborator

navidcy commented Jun 23, 2023

Let's check the docs preview before we merge (just in case).

@navidcy navidcy changed the title Update Oceananigans/KernalAbstractions versions Update Oceananigans/KernelAbstractions versions Jun 23, 2023
@navidcy
Copy link
Collaborator

navidcy commented Jun 23, 2023

Docs look good! Merge?

@navidcy
Copy link
Collaborator

navidcy commented Jun 24, 2023

[Note: Perhaps also deal with #112 before registering v0.2?]

@johnryantaylor
Copy link
Collaborator

removing the NPZD example is fine since as Jago says an example of using this model is in the quickstart anyway (and the NZPD model is pretty straightforward)

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jun 24, 2023

Great, yeah will merge #112 first. Thank you!

@jagoosw jagoosw merged commit f83a4df into main Jun 24, 2023
@jagoosw jagoosw deleted the jsw/update-dependencies branch June 24, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants