-
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
Support Oceananigans v0.95 and bump v0.13.4 #233
Conversation
I think I'm seeing a couple of different errors pointing to this function function intercept_tracer_tendencies!(model, intercepted_tendencies)
for (name, field) in enumerate(intercepted_tendencies)
field .= Array(interior(model.timestepper.Gⁿ[name + 3]))
end
end in I'm skipping the sediment tests for now just to see if the rest of the tests pass on CI (they did on my machine). @jagoosw I hope you don't mind that I re-organized |
That is quite strange, the changes since 0.93 shouldn't have effected any of this right? |
Yeah I agree and I couldn't find any hints in the diffs or the changelog... I'll try to take a closer look later today. |
Because of CliMA/Oceananigans.jl#3930 I had to rename I think there's a wider discussion to be had around supporting hydrostatic RK3 time stepping with OceanBioME.jl but I'll open a new issue for it. |
Hmmm, new error seems to be related to |
Let's try this again. Right now we're depending on the Docs are failing because they're pulling in v0.92.x based on the |
I think we're also encountering CliMA/Oceananigans.jl#3964 here. |
Updating to Oceananigans v0.95 now! |
Updating to Oceananigans v0.95.6 now! Need to dig more but PR CliMA/Oceananigans.jl#4008 might be related to the failing tests? |
Hi @ali-ramadhan, Sorry, I've not been keeping on top of this. I've had a go at refactoring the sediments, do you think it would be best to merge this PR with sediments broken and put that in a different PR, or do it all here? |
Hey @jagoosw! It is just Maybe the test will just pass with your refactor? Happy to merge this PR and find out. If the test still fails I'm happy to help debug. I just left a |
Just for reference here's the error I'm getting with
|
Docs are fine without |
Okay since its going to be a pretty major refactor so its less of a mess I'll do it in a different PR, should we add a warning to the sediment model constructors that they're broken? |
Good idea. I just added extra warnings for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for sorting this out, I'll fix the sediment imminently
🤞 that everything works out of the box!