uDALES / u-dales

uDALES: large-eddy-simulation software for urban flow, dispersion and microclimate modelling
https://udales.github.io/u-dales
GNU General Public License v3.0
44 stars 15 forks source link

Clean up and restructure namoptions #40

Closed bss116 closed 4 years ago

bss116 commented 4 years ago

The current namoptions sections can be made more clear and some parameters may be outdated. In this issue we can discuss how to clean up and restructure the namoptions.

Some initial suggestions:

bss116 commented 4 years ago

Issue #48 is also part of this and should be addressed latest in 0.2.0. For future development: @samoliverowens and I further discussed that we should have a way to keep the namoptions.xxx files tidy with only the relevant parameters present. This could be for instance by adding a routine that checks changes in namoptions against the default parameter set and eliminates all lines that are not necessary. However this only makes sense if parameters are changed within the pre-processing routine -- otherwise the parameter would have to be manually added to namoptions.xxx for every change, which is not really an improvement. This needs further discussion in the future.

samoliverowens commented 4 years ago

The value of nblocks is written to namoptions by the da_inp bash script, so I think it does need to stay in namoptions, but its value can be empty because it is not used to generate blocks.inp.xxx.

On this topic, there are three methods of generating blocks.inp.xxx, and I have set up &INPS in the following way to carry these methods out.

  1. Using an existing blocks file, containing just the buildings and no floors. The file name could be also be specified?
  2. Using one of the switches: lcube (linear cubes), lcastro (staggered cubes), lblocks (infinite canyons - possibly should be renamed to lcanyons). In order to have no buildings, I have made it necessary to use the lflat switch, with the assumption that specifying none of these switches means using method 1. Alternately, we could have a lblocksfile switch specifying to use method 1.
  3. Using LIDAR data. In this case the switch is llidar, and if specified the user need to specify the other parameters needed for the pre-processing. Again, the file name could be specified in namoptions.

It should be possible to only require the minimal number of &INPS parameters by writing the pre-processing routines well, but I can't really say what's best to do for the other headers because I have very little experience yet of fiddling with the parameters. For now I think the priority with respect to namoptions is producing some basic documentation of the available parameters - I can do this for &INPS, @bss116 are you ok to do this for what you're familiar with? And then at a later stage we can discuss ways of trimming it down?

bss116 commented 4 years ago

I had a quick look at the code, parameter nblocks is actually being used when reading in the blocks.inp.xxx file, so it is needed at the moment.

Adding a lblocksfile switch is a good idea. I think either is okay, to specify the path within script or in the &INPS section.

For documentation I would actually keep the things separate: one section for &INPS and the use of the pre-processing routines, and another for the FORTRAN input parameters (i.e. all other sections). We want to make clear that one is the direct input to the code, and the other one is the input to produce the additional inps input files. It is important to emphasise that changing anything in &INPS still requires to run the pre-processing routines, whereas changing any of the other parameters does not. I am happy to add information to the FORTRAN namelist parameters for all the ones I am familiar with.

dmey commented 4 years ago

Most of the list options are already well documented in DALES ( see https://github.com/dalesteam/dales/blob/master/utils/doc/input/dales-manual.pdf). So why simply report the ones we share also in our docs and add the new/missing ones?

bss116 commented 4 years ago

Yes that's true, I will do that.

samoliverowens commented 4 years ago

Ah it could be that the value of nblocks is used in the old pre-processing setup, but in fact the number of blocks is calculated first at the end of makeblocks, and then again at the end of createfloors, so the value specified in namoptions isn't actually needed. If having it in namoptions is confusing I will look into changing the da_inp bash script to add it rather than changing the value of it. This is all also true for nfcts.

Regarding documentation, the pre-processing uses parameters in headers other than &INPS, so maybe I should also document those and how they affect the pre-processing specifically?

dmey commented 4 years ago

Regarding documentation, the pre-processing uses parameters in headers other than &INPS, so maybe I should also document those and how they affect the pre-processing specifically?

For me, that would be ideal. 👍

bss116 commented 4 years ago

Ah it could be that the value of nblocks is used in the old pre-processing setup, but in fact the number of blocks is calculated first at the end of makeblocks, and then again at the end of createfloors, so the value specified in namoptions isn't actually needed. If having it in namoptions is confusing I will look into changing the da_inp bash script to add it rather than changing the value of it. This is all also true for nfcts.

What I meant is that nblocks is used at the startup of the program to determine the blocks (https://github.com/uDALES/u-dales/blob/7022e59b294ad51b05a1344985b14e411b51f796/src/modstartup.f90#L1730). This number must match what is in the blocks.inp file and therefore it also needs to be calculated and updated in the pre-processing. It would be less error-prone to not have this and just read in blocks.inp, but this requires some re-writing in how fortran reads in the input files.

bss116 commented 4 years ago

Regarding documentation, the pre-processing uses parameters in headers other than &INPS, so maybe I should also document those and how they affect the pre-processing specifically?

For me, that would be ideal. 👍

Yes, agreed. It would be good if you mention all the parameters used or changed by the pre-processing. I will document their definition and purpose in the main code together with all other namoptions (linking to the original dales documentation where possible).

samoliverowens commented 4 years ago

This number must match what is in the blocks.inp file and therefore it also needs to be calculated and updated in the pre-processing. It would be less error-prone to not have this and just read in blocks.inp, but this requires some re-writing in how fortran reads in the input files.

I think this is all taken care of when using the da_inp bash script. This script calls da_inp.m (which writes blocks.inp), then counts the number of lines in blocks.inp, and sets the value of nblocks in namoptions to the number of lines - 2 (accounting for the header).

bss116 commented 4 years ago

This number must match what is in the blocks.inp file and therefore it also needs to be calculated and updated in the pre-processing. It would be less error-prone to not have this and just read in blocks.inp, but this requires some re-writing in how fortran reads in the input files.

I think this is all taken care of when using the da_inp bash script. This script calls da_inp.m (which writes blocks.inp), then counts the number of lines in blocks.inp, and sets the value of nblocks in namoptions to the number of lines - 2 (accounting for the header).

Yes exactly, and as long as that works it's fine. Ideally we would not need to set this parameter at all (i.e. dales reading the full block file without telling it how many lines it has), but that belong to code improvement that is not priority at the moment.

samoliverowens commented 4 years ago

@bss116 the parameters in namoptions don't have to be specified in any particular order do they? I'm thinking of just changing da_inp so that it simply writes nblocks under the &DOMAIN header (and nfcts under the &ENERGYBALANCE header).

bss116 commented 4 years ago

They have to be in the correct section (e.g. &DOMAIN), but the order does not matter. Have a look here: https://github.com/uDALES/u-dales/blob/053dca71a5c013bc585676604cacd25b53da699e/src/modstartup.f90#L71. I'm pretty sure it is just a matter of changing these lines of code when we want to change the section they appear in. I suggest you make a list of changes you want to do, and then we discuss them here. I think for instance it would be good to move nfcts out of &ENERGYBALANCE, because it needs to be correctly set even in the case of lEB = .false..

bss116 commented 4 years ago

We should also check for consistency with the original dales before changing the parameter sections: https://github.com/dalesteam/dales/blob/9299a01e3b05c660e8a4b1fbb8f7066814dcfc13/src/modstartup.f90#L84.

bss116 commented 4 years ago

@ivosuter @samoliverowens I started a new branch for cleaning unused parameters in namoptions: https://github.com/uDALES/u-dales/tree/bss/namoptions-cleanup. I also added a new file (https://github.com/uDALES/u-dales/blob/bss/namoptions-cleanup/examples/namoptions.xxx) that lists all current parameters and their default options. There are a few things to discuss here:

1) There are some additional parameters compared to the original DALES (https://github.com/dalesteam/dales/blob/9299a01e3b05c660e8a4b1fbb8f7066814dcfc13/utils/doc/input/Namoptions.pdf). We should discuss which of these are actually being changed and if we would always use the same value, we should remove them from namoptions (can be added individually when they need to be parameters for developing a new feature). This is mainly in the section &PHYSICS.

2) There are some parameters introduced by Jasper Tomas' work, and I am not sure all of these still have full functionality. Whatever is not working, we should either remove completely or at least remove the parameter from namoptions and indicate in the code that it needs further development. These are particularly in the sections &RUN and &INLET.

3) there are some parameters we have introduced in our code development, and I think some of these are also outdated or not required any more. This concerns mainly the sections &BC and &INLET.

4) for those parameters that are new compared to the original DALES, we need to discuss default values.

I will start with some of the things I noticed when writing the namoptions.xxx sample file.

bss116 commented 4 years ago

ad 4)

in &RUN we have the parameters:

In &PHYSICS there are the parameters:

bss116 commented 4 years ago

ad 3) @ivosuter

in &BC:

samoliverowens commented 4 years ago

@bss116 I think you're right about the wall functions - whether they are used is determined by iwall..., and if they aren't (iwall...=1), the valuesbctf... are needed, so maybe lwallfunc is/should be redundant now.

wttop/thl_top e.g. are Neumann/Dirichlet BCs for the top, corresponding to BCtopT = 1/2.

wtsurf/thls e.g. are for Neumman BCs/wall functions at the bottom, corresponding toBCbotT = 1/2. @ivosuter I'm note sure what happens for cells just above the ground - are they updated according to BCbotT (and wtsurf/thls) or iwalltemp (and bctfz)?

I think ltempeq and lbuoyancy defaulting to false is fine!

ivosuter commented 4 years ago

@bss116 -lwallfunc is used in modinlet, otherwise it's deprecated. Can be removed from my side. -libm, in theory you could run the LES without any obstacles.. but it is kind of useless at this point -lles=false bypasses the subgrid models, I don't think its very useful and the corresponding code in modsubgrid can also be deleted (it is also not very hard to reimplement if someone ever wants to)

-lbuoyancy/ltempeq/lmoist. kind of important switches so the default is not really that relevant, i would recommend keeping them in the nameoptions at all time

-wsvsurfdum/wsvtopdum (dum=dummy) are they even read from nameoptions? I guess we need some values for scalar boundary conditions and we chose a default of 0, resulting in zero-flux at the top and bottom (since BCtops=1 and BCbots=1). If we keep the dummies the scalar boundary conditions are defined the same way as q and T so I would probably keep them.

-bctfxm etc.. yes BounaryConditionTemperatureFluxXMinus, if anybody wants to prescribe a flux instead of a fixed value with wall function. Definitely keep those.

-subroutine 'bottom' still exists. Bottom is currently still always called but does essentially nothing if the floor is covered with blocks (velocity difference is basically 0 (->very small flux) and internal velocities and temperatures in blocks (ibmnorm/ibmwallfun) will overwrite whatever 'bottom' predicts). In theory one does not have to cover the floor with blocks and could use the subroutine bottom (to cover the floor is only necessary when one wants to use the energy balance). The wallfunction has the cases 91 and 92 for bottom momentum and heat transfer. As long as 'bottom' exists we need wtsurf (temperature flux at bottom), thls (temperature at bottom) and wqsurf (moisture flux at bottom). qts is used in modthermodynamics to get a BC for the moisture profile. thl_top and wttop, qt_top, wqtop are needed for top boundary conditions (temperature and humidity at the top boundary, respectively the temperature and moisture flux at the top boundary)

ivosuter commented 4 years ago

about default values you should probably discuss. I'm not sure if choosing the value one would expect is always the correct choice I think the default value should be the ones that are almost always being used, and then one should probably remove them from the namelist to reduce clutter if the value is in the namelist, changes often and depends from case to case I would chose a default which will very likely break the simulation or becomes obvious quickly if one deletes it from the namelist by mistake e.g.: -the default for concentrations should probably be a negative number -the default for tempeqshould probably be false, since one wouldn't necessarily check temperature if one believes to run a neutral simulation. On the other hand, one would probably spot a homogenous temperature field quickly when running a presumably buoyant case

bss116 commented 4 years ago

@ivosuter thanks!

-wsvsurfdum/wsvtopdum (dum=dummy) are they even read from nameoptions? I guess we need some values for scalar boundary conditions and we chose a default of 0, resulting in zero-flux at the top and bottom (since BCtops=1 and BCbots=1). If we keep the dummies the scalar boundary conditions are defined the same way as q and T so I would probably keep them.

They are currently declared in the &BC namelist. I will then leave this as it is for now, as I am not sure what's the state with the scalars in the code anyway...

-subroutine 'bottom' still exists. Bottom is currently still always called but does essentially nothing if the floor is covered with blocks (velocity difference is basically 0 (->very small flux) and internal velocities and temperatures in blocks (ibmnorm/ibmwallfun) will overwrite whatever 'bottom' predicts). In theory one does not have to cover the floor with blocks and could use the subroutine bottom (to cover the floor is only necessary when one wants to use the energy balance). The wallfunction has the cases 91 and 92 for bottom momentum and heat transfer. As long as 'bottom' exists we need wtsurf (temperature flux at bottom), thls (temperature at bottom) and wqsurf (moisture flux at bottom). qts is used in modthermodynamics to get a BC for the moisture profile.

Okay I see! Yes now I remember that we had this discussion whether we should leave it in or not. I guess the advantage of taking it out would be to have the code always doing the same thing and simplifying the namoptions. The advantage of keeping it is that for non energy balance simulations the input files (blocks, facets) could be much simpler and the code would also be closer to the original DALES. We should probably discuss this with everyone at some point.

bss116 commented 4 years ago

I think the default value should be the ones that are almost always being used, and then one should probably remove them from the namelist to reduce clutter if the value is in the namelist, changes often and depends from case to case I would chose a default which will very likely break the simulation or becomes obvious quickly if one deletes it from the namelist by mistake

Yes this makes a lot of sense! We should check the default values against these criteria.

bss116 commented 4 years ago

I made a list of all current parameters with default values and suggest we use this for further discussions. Parameters without default values are commented out (using ! in fortran namelists). If you are happy with any of the default parameters please tick them off the list.

&RUN

&DOMAIN

&BC

&ENERGYBALANCE

&INLET

&WALLS

&PHYSICS

&DYNAMICS

&NAMSUBGRID

&NAMCHECKSIM

samoliverowens commented 4 years ago

@ivosuter just to follow up on this:

-subroutine 'bottom' still exists. Bottom is currently still always called but does essentially nothing if the floor is covered with blocks (velocity difference is basically 0 (->very small flux) and internal velocities and temperatures in blocks (ibmnorm/ibmwallfun) will overwrite whatever 'bottom' predicts). In theory one does not have to cover the floor with blocks and could use the subroutine bottom (to cover the floor is only necessary when one wants to use the energy balance). The wallfunction has the cases 91 and 92 for bottom momentum and heat transfer. As long as 'bottom' exists we need wtsurf (temperature flux at bottom), thls (temperature at bottom) and wqsurf (moisture flux at bottom). qts is used in modthermodynamics to get a BC for the moisture profile. thl_top and wttop, qt_top, wqtop are needed for top boundary conditions (temperature and humidity at the top boundary, respectively the temperature and moisture flux at the top boundary)

At what level are the floors located?

Is there a layer of cells below the floors that the bottom BC is being applied to? If this is the case, what is this effect of using the default value of thls = -1 - won't there be a huge temperature difference between the floor facets and the cells below?

Or else are the floors located at the lowest level, and the value predicted for them by bottom is simply overwritten by zwallfun?

ivosuter commented 4 years ago

-the normal floor is at height h=0. If you add blocks as floor instead, for radiation etc.., they are centered at zf(0) with the bottom edge touching the ground (h=0) and the top edge at 0+dzf

Capture

p.s. the index for height coordinates starts at 0 for whatever reason

ivosuter commented 4 years ago

there are ghostcells below the ground, yes. They are also at the top and at the horizontal boundaries... thls=-1 would probably break the simulation since then the ground is at -1 kelvin (wheter it is used depends on BCbotT of course), I assume thls is also specified in the namelist?

the fluxes predicted by bottom go INTO the block for roads etc, the flux out of the block (into the fluid) is calculated by zwallfun. Also the prognostic variables for interior temperature and velocities of the blocks are overwritten in ibmnorm

samoliverowens commented 4 years ago

Thanks @ivosuter, that helps a lot. thls is indeed in the namelist, I think it's sensible to just set the default to 288K - I don't know if -1 would actually break the simulation.

I'm going to quickly write up some documentation on setting the boundary conditions, as some final year project students are starting to run simulations and it'll make it easier to explain. @bss116 do you think I should start a new branch for this, or do it in bss/namoptions-cleanup?

bss116 commented 4 years ago

Yes, I'd say for documentation of namoptions it's best to use the bss/namoptions-cleanup branch. I was in fact planning to add some documentation to the switches this afternoon. If you are documenting the &BC section, then I'll start with the other ones?

By the way, as a general workflow I was thinking the following:

  1. namoptions clean-up + description
  2. setting up example simulations with only non-default values in namoptions.inp
  3. testing the pre-processing with these examples
samoliverowens commented 4 years ago

Ok great, yes I'll start with &BC. That workflow sounds good!

bss116 commented 4 years ago

@ivosuter I'm just writing the advection scheme description. Our options are: 1 = first order upwind, 2 = central difference, 7 = kappa advection. In advection.f90 the cases to choose from are: mom : 2 tke: 2 (only if loneeqn = True) thl : 2, 7 qt: 2 sv : 1, 2, 7. Is this correct?

ivosuter commented 4 years ago

No one has used oneeqn subgrid model in ages :D Consider removing it? Is upwind scheme useful? Yes, 2 = 2nd order central difference, 7 = flux limited for scalars

bss116 commented 4 years ago

Okay, I'll make a note about that. Do you mean option 7 only works for scalars, or scalars only work with option 7?

ivosuter commented 4 years ago

neither, it could potentially work both ways. But scalars should never be negative (not so relevant for velocity), that's why special treatment is necessary near steep gradients in the concentration field-> kappa-scheme. The disadvantage is that the kappa advection scheme is dissipative, it 'smears out' the concentration field. It is also slower than cd2. Not sure why we don't use it for moisture though.. we probably should?

bss116 commented 4 years ago

Ok I see, thanks! Yeah it sounds like we should also have at least the option for moisture. I'll put it on the list of things to discuss...

bss116 commented 4 years ago

@ivosuter In PHYSICS we have the parameters z0, zh0, numol and prantlmol. z0 is the only one that is originally in DALES, where it has the default -1 . In our code the default has changed to 0.1, and also zh0=0.1. numol and prantlmol have no default values at all. In the example simulations the values are: z0 = 0.01 z0h = 0.000067 numol = 1.5e-5 prantlmol = 0.71

What would you suggest as default values? Should numol, prantlmol be parameters at all?

z0, z0h are only used by the subroutine bottom, if I'm not mistaken? Is it easy to implement to switch off bottom completely if the floor is covered with blocks and the matching boundary conditions are applied? Then we could reset the default values for everything concerning the subroutine bottom to the defaults in DALES, i.e. -1 for most variables, and the code would break if either the new or the old way of treating the ground floor is not set up correctly.

ivosuter commented 4 years ago

Yes, facets all have their own roughnesses. So, as long as "bottom" exists z0h and z0 need to stick around, otherwise you could get rid of them too. Sure, you can chose the default of -1 for z0 and z0h, the lines log(delta/z0) would then indeed throw an error in the case z0 or z0h are not set correctly.

Maybe remove numol and prandtlmol from the namelist? And just set "real, parameter :: numol = 1.5e-5" "real, parameter :: prandtlmol = 0.71" ? numol is of course temperature dependent but for our case a global value of 1.5e-5 should be fine. Prandtl 0.71 is often used in LES and if someone really needs to change this, it will not be hard to either change the parameter value or reintroduce it to the namelist.

bss116 commented 4 years ago

Okay, I will remove numol and prandtlmol from the namelist and set the defaults in the code.

What do you think about setting the parameters used only in bottom to the defaults from the original DALES (z0 = -1, thls = -1, wtsurf = -1, wqsurf = -1), set z0h = -1 and move them all back to &PHYSICS? Then raise an issue that we need to implement to call bottom only if the floors are not covered.

I was thinking of introducing new sections:

What do you think about these?

bss116 commented 4 years ago

The overview of the namoptions can be found here: https://github.com/uDALES/u-dales/blob/bss/namoptions-cleanup/docs/udales-namoptions-overview.md. I will create a pull request shortly.

bss116 commented 4 years ago

There are a few points left for discussion, for some of them it is probably easiest to address when we next meet up (on skype):

&RUN

&BC

&WALLS

&PHYSICS

&DYNAMICS

&NAMSUBGRID

ivosuter commented 4 years ago

@bss116 lwalldist is the swtich to calculate wall effects for the subgrid models. It has not been used in a long time since we never used smagorinsky or one equation subgrid model anymore. I don't know wether it is still fully functional. About soil moisture:

So, lconstW tells you whether the soil moisture should be kept constant or depend on the latent heat flux

tomgrylls commented 4 years ago

Trying to catch up on all this - all looks good and very helpful for future users of the code. Some initial notes:

bss116 commented 4 years ago
  • I am going to merge my last branch of the code with cleaned-up and more developed modules for scalars, trees, purifiers and statistics. The effect of this change will be that many of the parameter names change but if we have sections &TREES, &PURIFS etc. this will be easy to implement here immediately.

Then it might be the best to comment out all the scalar, trees, purifiers and inlet parameters for pull request #64? If they are potentially malfunctioning then it is better to say they are currently not supported and we just add the parameters and documentation for these after you merged your branch.

tomgrylls commented 4 years ago

I am meeting @dmey at 1300 to discuss this merge so I think I will be able to do it quite soon. They are not malfunctioning in that version they are just not the latest versions. If you comment these out in the code then the code will not compile unless we strip out all of their functionality. I assume it will be best to close #64 before merging so we can do this as it is and then I update these during the merge.