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

Be more concrete #282

Merged
merged 9 commits into from
Jul 6, 2019
Merged

Be more concrete #282

merged 9 commits into from
Jul 6, 2019

Conversation

ali-ramadhan
Copy link
Member

Reopening a PR based on #261 and #263 which I reverted as it broke some of the examples we usually run, e.g. free convection.

Before merging we should probably either refactor the examples to use the new boundary conditions API or come up with this BoundaryConditionsWizard that does everything for us.

@ali-ramadhan
Copy link
Member Author

Do these changes mean that we need to rewrite the PR that makes things more concrete? It looks like there will be a lot of merge conflicts.

Originally posted by @glwagner in #290 (comment)

Looks like there will be a few conflicts but mostly small changes to the boundary conditions will need to be made. They're still mutable structs in this PR so a lot of the examples should still work.

I think the two big issues with this PR are still the same:

  1. The way we set boundary conditions changes a little bit in this PR so we should probably also update any existing examples and scripts.
  2. This PR breaks checkpointing. This is fine as long as we have a fix ready to merge. Been meaning to convert to a native Julia checkpointer that only saves the bare essentials to disk (either in binary or JLD2) so this is a good opportunity.

@glwagner
Copy link
Member

I think we should let the checkpointing functionality lapse and return to it later, rather than keeping this important PR on hold until we solve that difficult problem.

@ali-ramadhan
Copy link
Member Author

The checkpointing is important for long simulations like the seasonal cycles we've been running.

I'm okay with merging this soon as long as checkpointing becomes a priority afterwards.

It's not a difficult problem. You just pick the dozen or so fields that you need to checkpoint, save them, and have a pickup or restore function that reads the fields from disk and initializes a new model.

@glwagner
Copy link
Member

Looks like the examples all work. What needs to be updated?

@ali-ramadhan
Copy link
Member Author

I'll nuke checkpointing for now, resolve the merge conflicts, and merge this in.

@ali-ramadhan
Copy link
Member Author

Not sure why GitLab CI pipeline isn't running on this PR (maybe because it's from a fork?) but the GPU tests pass on Engaging so I'll merge this PR once Travis and Appveyor pass as well.

@codecov
Copy link

codecov bot commented Jul 6, 2019

Codecov Report

Merging #282 into master will decrease coverage by 10.53%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #282       +/-   ##
===========================================
- Coverage   72.57%   62.04%   -10.54%     
===========================================
  Files          24       24               
  Lines        1032     1009       -23     
===========================================
- Hits          749      626      -123     
- Misses        283      383      +100
Impacted Files Coverage Δ
src/Oceananigans.jl 62.5% <ø> (-37.5%) ⬇️
src/output_writers.jl 74.57% <ø> (-6.82%) ⬇️
src/models.jl 89.28% <100%> (-4.47%) ⬇️
src/boundary_conditions.jl 66.12% <100%> (-1.22%) ⬇️
src/fieldsets.jl 100% <100%> (ø) ⬆️
src/poisson_solvers.jl 39.16% <0%> (-60.01%) ⬇️
src/utils.jl 26.08% <0%> (-26.09%) ⬇️
src/time_steppers.jl 74.17% <0%> (-7.14%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fdff75...d8da673. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jul 6, 2019

Codecov Report

Merging #282 into master will decrease coverage by 10.53%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #282       +/-   ##
===========================================
- Coverage   72.57%   62.04%   -10.54%     
===========================================
  Files          24       24               
  Lines        1032     1009       -23     
===========================================
- Hits          749      626      -123     
- Misses        283      383      +100
Impacted Files Coverage Δ
src/Oceananigans.jl 62.5% <ø> (-37.5%) ⬇️
src/output_writers.jl 74.57% <ø> (-6.82%) ⬇️
src/models.jl 89.28% <100%> (-4.47%) ⬇️
src/boundary_conditions.jl 66.12% <100%> (-1.22%) ⬇️
src/fieldsets.jl 100% <100%> (ø) ⬆️
src/poisson_solvers.jl 39.16% <0%> (-60.01%) ⬇️
src/utils.jl 26.08% <0%> (-26.09%) ⬇️
src/time_steppers.jl 74.17% <0%> (-7.14%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fdff75...d8da673. Read the comment docs.

@ali-ramadhan ali-ramadhan merged commit bda3fdb into CliMA:master Jul 6, 2019
arcavaliere pushed a commit to arcavaliere/Oceananigans.jl that referenced this pull request Nov 6, 2019
Be more concrete

Former-commit-id: bda3fdb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants