Victor-Garcia-p / DWF-Model

Undergraduate thesis project for Marine Sciences degree at University of Barcelona
0 stars 0 forks source link

suggested changes to make it more flexible #18

Closed rogersamso closed 1 year ago

rogersamso commented 1 year ago

The changes I suggest should make your code more flexible.

This implementation moves the code that does not depend on the arguments of the function outside of it. This way, this code is only run once (and not every time the DWF function is called).

It also sets all parameters by default (with keyword arguments), and the only thing you must pass to the DWF function is the WaterLayers. If what I coded works as expected (which I didn't have the time to check), the code should work for any number of layers (1, 2, 3 or more).

The prova.jl file is just to show you how to use the function with 3 layers and with default values for the function arguments.

However, the default values of the arguments to the DWF function can also be changed. For instance, you could launch multiple simulations in loop like this, changing the values of certain parameters (in this case u₁₀ and dTdz) :

include("model_loop_v2.jl")

SW_layer = WaterLayer(10.0, 37.95, 13.18)
LIW_layer = WaterLayer(20.0, 38.54, 13.38)
WMDW_layer = WaterLayer(grid.Lz, 38.41, 12.71)

layers = [SW_layer, LIW_layer, WMDW_layer]

keyword_arguments = [Dict(:u₁₀=>10, :dTdz=>0.01), 
                                          Dict(:u₁₀=>15, :dTdz=>0.02),
                                          Dict(:u₁₀=>20, :dTdz=>0.03)
                                          ]

for kwargs in keyword_arguments
    DWF(layers...; kwargs...)
end

I still think the DWF function does too many things, though. I would split it this way:

  1. a function called build_model (or similar) that returns the model with all set fields (from lines 104 to 178)
  2. another function called prepare_simulation that takes the code from lines 185 to 240 which returns the Simulation object.
  3. Then finally you would use the run! method, passing the Simulation object.

By the way, remove all unnecessary comments in the code

rogersamso commented 1 year ago

@Victor-Garcia-p, I took the time to make those improvements myself. So now the code is split in the functions I suggest above.

Check that you obtain the same results that you got with your version, before merging these changes.

Victor-Garcia-p commented 1 year ago

@rogersamso First of all, thanks for your time. The model is clearer now than before and I think it could be great to use your version of it because its easy to make loops. The only problem is that the model does not work properly (ill send you a video). The main problem is the wind, even if we put u(10)= 10m/s the model does not represent the effect of wind in the surface, it does, however, represent correctly the convection between water masses due to differences of density. I thing we could try to make the model make the same simulation as the example of oceananigans, to do so it’s enough to put S=35 in all layers and leave u(10)= 10m/s. (https://clima.github.io/OceananigansDocumentation/v0.45.2/generated/ocean_wind_mixing_and_convection/)

For now, I will focus on the calculation of Brunt Vaisala and writing the TFG, because although my version of the model is chaotic and does not work for loops it can replicate the wind effect. When I finish this I will try to solve the problem, but I don’t think is a priority now.

rogersamso commented 1 year ago

OK @Victor-Garcia-p , I'll have a look at what may be wrong. Can you tell me which script you use to make the videos?

This in your code does not look right to me, because instead of passing the salinity field, which depends on z, you are passing only the initial salinity of only the top layer.

    @inline Qˢ(x, y, t, S, evaporation_rate) = -evaporation_rate * S_WM[1] # [salinity unit] m s⁻¹

Apart from that, I could not find any differences between your code and mine. Please note that the function signature is now different than your DWF function, and the u10 needs to be passed as a keyword argument. If not passed, the default value is 10. See the prova.jl file for an example.

Victor-Garcia-p commented 1 year ago

@rogersamso the salinity evaporation is only on the upper layer, then it makes sense that it occurs only in the SW[1]? But i understand what you say, maybe its an error.

I have used "3WM__u₁₀=10_S=37.95-38.54-38.41_dTdz=0.01_T=13.18-13.38-12.71_dim=2D_run=true" as an example. I think is better to compare with the original model than to mine’s.

rogersamso commented 1 year ago

But i understand what you say, maybe its an error.

ok, leave it as is in the original model, please.

I have used "3WM__u₁₀=10_S=37.95-38.54-38.41_dTdz=0.01_T=13.18-13.38-12.71_dim=2D_run=true" as an example. I think is better to compare with the original model than to mine’s.

Ok, but I need to know how to make the plots, using your code. Which file do I need to execute?

Victor-Garcia-p commented 1 year ago

@rogersamso It's Movie.jl file with

load_variable( "3WM__u₁₀=10_S=37.95-38.54-38.41_dTdz=0.01_T=13.18-13.38-12.71_dim=2D_run=true", )

I will send you the file by email so that you don't need to run the simulation. Add the file to "data" folder

rogersamso commented 1 year ago

We will need to discuss the code in Movies.jl, because not only do you use it to plot, but also to define parameters, which is not what I would expect.

wlims = (-0.05, 0.05)
Tlims = (12.7, 13.40)
Slims = (37.8, 38.53)
νₑlims = (1e-6, 5e-3)

These should be inferred from the layers you defined to build the model, and not hardcoded in the plotting functions.

Anyway, I will try to have a look at it asap.

Victor-Garcia-p commented 1 year ago

Yes, it would be nice to improve this. But for now I will focus on writing and Brunt Vaisala

El lun., 30 ene. 2023 12:07, Roger Samsó @.***> escribió:

We will need to discuss the code in Movies.jl, because not only do you use it to plot, but also to define parameters, which is not what I would expect.

wlims = (-0.05, 0.05)

Tlims = (12.7, 13.40)

Slims = (37.8, 38.53)

νₑlims = (1e-6, 5e-3) ``

These should be inferred from the layers you defined to build the model, and not hardcoded in the plotting functions.

Anyway, I will try to have a look at it asap.

— Reply to this email directly, view it on GitHub https://github.com/Victor-Garcia-p/TFG/pull/18#issuecomment-1408422280, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZVQRRJJBZOZSJD3IDF6HK3WU6OG5ANCNFSM6AAAAAAUE7YWFA . You are receiving this because you modified the open/close state.Message ID: @.***>