ali-ramadhan / Atmosfoolery.jl

Compressible non-hydrostatic model built on top of Oceananigans.jl so it runs on CPUs and GPUs
MIT License
3 stars 0 forks source link

Consolidate verification experiments, add regression tests, standardize operators, and add lazy fields #63

Closed ali-ramadhan closed 4 years ago

ali-ramadhan commented 4 years ago

Made some changes to the main code:

  1. Updated to Oceananigans.jl v0.23.0.
  2. Fields now have boundary conditions so no need for dirty hacks in time_stepping.jl anymore (but still need to add a proper boundary_conditions kwarg to CompressibleModel, easy to do though).
  3. time_step.jl takes a single time step now like Oceananigans.jl. This allows JULES to use the Oceananigans.Simulation type to handle time stepping (verification experiments use it now).
  4. models.jl -> compressible_model.jl
  5. Added show methods.
  6. No more model.parameters as we're moving to more local parameters in Oceananigans.

But I found the code a little fragile and easy to break so instead of doing more refactoring I ended up consolidating the verification experiments into 1 file per experiment. The simulations are set up by functions with the thermodynamic variable and end time as input arguments. This allows us to merge the energy and entropy versions and allows the experiments to be easily reused for regression tests.

I added regression tests for the 1 gas and 3 gas versions of the rising thermal bubble (using energy and entropy for both).

@thabbott Let me know what you think but I was thinking we could merge this into #60, merge #60 into master, and then tag a v0.2.0 for the n-density dry model.

I spent a while trying to figure out why the thabbott/multiple_components branch became slow and narrowed it down to three different spots where it was allocating lots of memory but for some reason couldn't figure out what the problem was (might have been so obvious I missed it lol). But anyways, I decided that it might be easier to revert back to a good commit.

So some of the new things we added (namely thermodynamic_fields and tracer_fields) were taken out. But with performance benchmarks and regression tests I can copy-paste them back in and make sure nothing breaks or slows down in the process. I can do that in this PR (or #60).

codecov[bot] commented 4 years ago

Codecov Report

Merging #63 into thabbott/multiple_components will not change coverage by %. The diff coverage is n/a.

Impacted file tree graph

@@                      Coverage Diff                      @@
##           thabbott/multiple_components      #63   +/-   ##
=============================================================
  Coverage                         94.91%   94.91%           
=============================================================
  Files                                11       11           
  Lines                               295      295           
=============================================================
  Hits                                280      280           
  Misses                               15       15           

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 dbc4596...dbc4596. Read the comment docs.

ali-ramadhan commented 4 years ago

95% code coverage :rocket: Just gotta touch show(::CompressibleModel) and acoustic_cfl for 100%.

ali-ramadhan commented 4 years ago

So I ended up doing a lot of refactoring, mostly to standardize variable names and function signatures a bit, especially for operators. We've been bitten hard by not doing this in the past...

Some changes:

  1. Introduced a legend for common variables used in operators and time stepping (tilde means a named tuple of fields, no vector means one field)

    ρ̃  -> density fields
    ρũ -> momentum fields
    ρc̃ -> conservative tracer fields
    K̃  -> diffusivity fields
    ũ  -> velocity fields
    c̃  -> tracer fields

    Maybe it'll turn out to be a bad idea to use so much unicode + combining characters.

  2. Re-ordered the CompressibleModel struct and I try to follow this order in function signatures.

    mutable struct CompressibleModel{A, FT, Ω, D, M, T, K, Θ, G, X, C, P, F, S, R, I, W} <: AbstractModel
              architecture :: A
                      grid :: Ω
                     clock :: Clock{FT}
             total_density :: D
                   momenta :: M
                   tracers :: T
             diffusivities :: K
    thermodynamic_variable :: Θ
                     gases :: G
                   gravity :: FT
                  coriolis :: X
                   closure :: C
              microphysics :: P
                   forcing :: F
             slow_forcings :: S
          right_hand_sides :: R
    intermediate_variables :: I
     acoustic_time_stepper :: W
    end

Now I feel much more comfortable implementing lazy fields lol.

ali-ramadhan commented 4 years ago

Added some simple lazy primitive fields from #60 (+ tests) which we can use to hopefully simplify a lot of the operators and interfacing with things like turbulence closures.

But I'm a little hesitant to use the lazy fields right now as I'm not sure if they're GPU performant in their current form (don't see why they won't be, but gotta test...). But with regression tests and performance benchmarks, should be straightforward to refactor them until they are GPU performant.

ali-ramadhan commented 4 years ago

Hmmm, one of the Travis builds failed but looks like it was a timeout, not an actual test failure.