Single structure

[swilliamson7]

Okay, I think this is the final pull request. It just makes everything use S, etc. etc. No Enzyme or Checkpointing yet, I'll make separate pull requests for those when I check everything still works on my computer. If you could ensure you can still run the integration with my changes that would be fabulous! I have double and triple checked on my end but I'm always afraid I'll miss something

[milankl]

Why does this have to be an Int?

[swilliamson7]

I think we just didn't want a Real because of type-instabilities. Is there a different option to Int here?

[milankl]

Float64?

[milankl]

Also note that because you change the defaults this is a breaking change, because you said "works as before" at some point 😉

[milankl]

Are there any tests you want to add for the changes you made? Cause "everything still works on my computer" isn't quite sufficient ;)

[swilliamson7]

That's fair lol, I don't think anything specific should be needed/added, the same tests as before should work since theoretically nothing changed

[swilliamson7]

Does the CI test mean something, or can I go ahead and merge this today?

[milankl]

Haha, no, please don't merge PR with failing tests (just changed this so that you cannot do this accidentally 😉), you can click on it to see what's going on, the first error is

  Expression: all(Prog.u .== 0.0f0)
  type Tuple has no field u
  Stacktrace:
   [1] getproperty(x::Tuple{ShallowWaters.ModelSetup{Float32, Float32}, ShallowWaters.PrognosticVars{Float32}}, f::Symbol)
     @ Base ./Base.jl:37
   [2] macro expansion
     @ /opt/hostedtoolcache/julia/1.9.4/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:478 [inlined]
   [3] macro expansion
     @ ~/work/ShallowWaters.jl/ShallowWaters.jl/test/runtests.jl:6 [inlined]

so you'd need to check what's going on here, maybe change the PR maybe change the tests.

[swilliamson7]

Ah okay, I missed that line in the error (all I noticed before was something about Julia actions and wasn't sure what that was). I'll look at what's going on

[milankl]

Awesome, thanks Sarah! I can look at this either tomorrow afternoon or maybe next week. But maybe you'll be succesful making all the tests pass in the meantime anyway 😉

[swilliamson7]

Awesome, thanks Sarah! I can look at this either tomorrow afternoon or maybe next week. But maybe you'll be succesful making all the tests pass in the meantime anyway 😉

Thanks for the faith in my debugging skills lol, but I'm rather confused at the moment. If you have one second I have a quick question! Do you have guesses on why in this test

@testset "Mixed Precision" begin
    Prog = run_model(Float32,Tprog=Float64)
    @test all(abs.(Prog.u) .< 10)    # velocity shouldn't exceed +-10m/s
    @test all(Prog.u .!= 0.0f0)

    Prog = run_model(Float16,Tprog=Float32,Ndays=50)
    @test all(abs.(Prog.u) .< 10)    # velocity shouldn't exceed +-10m/s
    @test all(Prog.u .!= 0.0f0)
end

My code passes the first where T=Float32 and Tprog=Float64, but fails the second where T=Float16 and Tprog=Float32? In my mind I should either fail both or pass both, but passing one of the two seems odd

[milankl]

Something blows up, (you can see the CI output no?)

  Got exception outside of a @test
  InexactError: Int64(NaN)

This could be related to you changing the model setup too (gravity is actual gravity again, not reduced gravity that Float16 maybe cannot handle as well). I'd suggests going back to the original physical setup here, and maybe to keep in mind that you use Float32 and the barotropic setup?

[swilliamson7]

Something blows up, (you can see the CI output no?)

I can indeed see that something blew up, I was more confused on why only one of the two did 🙃

This could be related to you changing the model setup too (gravity is actual gravity again, not reduced gravity that Float16 maybe cannot handle as well). I'd suggests going back to the original physical setup here, and maybe to keep in mind that you use Float32 and the barotropic setup?

You were correct that it happened because of something that I did to the default parameters, I didn't think about the fact that Float16 is a little more sensitive...

[milankl]

You were correct that it happened because of something that I did to the default parameters, I didn't think about the fact that Float16 is a little more sensitive...

Yes, because this model is branded as "works with 16-bit arithmetic" I'd really like to have a default model setup that works in Float16 out of the box. It's not surprising that things may need some more careful investigation when you change the model setup (which changes time step etc. too) whether this requries a compensated time integration etc etc, but the default setup should ideally just work with Float16...

[milankl]

I think this is also why #161 is hanging in there for years now because maybe that would give a bit more stability with Float16 but whether I'll ever get to this, I doubt it. All eyes are on SpeedyWeather now!!

[swilliamson7]

Yes, because this model is branded as "works with 16-bit arithmetic" I'd really like to have a default model setup that works in Float16 out of the box. It's not surprising that things may need some more careful investigation when you change the model setup (which changes time step etc. too) whether this requries a compensated time integration etc etc, but the default setup should ideally just work with Float16...

Totally fair and I completely understand! I should have considered what being compatible with Float16 implied for what parameters would be stable

[milankl]

It's really just about the defaults, you can obviously change this as you like!

[milankl]

Congratulations all tests pass 🎉 I also leave this up to you whether you'd want to merge or squash merge