-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
With the new kernel launching, do I need to do something to get the threads to sync now that there's no |
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) |
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 ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
current particles implimentation is really hard to maintain
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? |
I wan't part of setting that up -- not sure how to set the tests to run on your own machine. |
The examples |
Is there a reason the NPZD example is not included in the list of examples that are literated in the docs? |
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? |
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.mp4I 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. |
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. |
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 |
Let's check the docs preview before we merge (just in case). |
Docs look good! Merge? |
[Note: Perhaps also deal with #112 before registering v0.2?] |
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) |
Great, yeah will merge #112 first. Thank you! |
No description provided.