-
Notifications
You must be signed in to change notification settings - Fork 1
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
CBTCU v1.9 #15
CBTCU v1.9 #15
Conversation
@@ -10,25 +10,33 @@ local MU = level(HPLevelType.kMaximumThetaE,0) | |||
local HL = level(HPLevelType.kHeightLayer,500,0) | |||
local HG = level(HPLevelType.kHeight,0) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"local" could be used for these variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "local" actually do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it declares the variable as a local variable instead of global, as you may have guessed :D
Basically it is the way lua recommends to be done: always use "local" unless you have a reason not to. It also has some performance implications as fetching a variable from global scope is more expensive. I have not benchmarked this though and I guess it's rather small, unless running on a very tight loop.
himan-scripts/CB-TCU-cloud.lua
Outdated
return | ||
end | ||
|
||
CBlimit = 2000 --required vertical thickness [degrees C] to consider a CB (tweak this..!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment for these variables is a leftover from earlier commit?
Also in smarttool macro TCulimit is 1500 (not 1000).
res[i] = FlightLevel_(EL500[i]*100) | ||
if (EL500[i] - LCL500[i] > TCUlimit) then | ||
--we don't use vertical search for flight level of EL500 but calculate directly from EL500 pressure | ||
res[i] = FlightLevel_(pEL500[i] * 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clever 👍
In thing that caught my eye is that the smarttool function call is:
vertfl_findh(par3_meps,0,TopLim,EL500,1)
The search is capped to TopLim which is set to FL 650. Should this also be checked here, ie.
if res[i] > 650 then
res[i] = Missing
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is just there because smarttool function requires some value and FL650 is so high that EL500 will always be below. At least in our latitudes. And if there is 20km tall convection I think it's better to put it as it is than to turn it into missing.
himan-scripts/CB-TCU-cloud.lua
Outdated
@@ -64,10 +74,11 @@ for i=1, #EL500 do | |||
res[i] = Missing | |||
|
|||
--TCU | |||
if ((Tbase[i]-Ttop[i]>TCUlimit) and (NL[i]>0) and (CIN500[i]>CINlimTCU)) then | |||
res[i] = FlightLevel_(EL500[i]*100) | |||
if (EL500[i] - LCL500[i] > TCUlimit) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In smarttool the if condition is:
if ((EL500-LCL500>TCulimit) and (cl_meps>0) and (CIN500>CINlimTCu))
The two latter boolean conditions are missing
--Limit top value | ||
if (CAPE500[i]>math.exp(1)) then | ||
if (CAPE500[i] > math.exp(1)) then | ||
--Add for overshooting top based on CAPE | ||
res[i] = -(res[i] + CAPE500[i] / (math.log(CAPE500[i]) * 10)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In smarttool the last term (10) has a division operator, not multiplication. I tried to think if you are doing some kind of mathematical trick here but I couldn't get it :D
I don't understand why Simo is using exp(1) here, it's just a constant number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put () around it which is the mathematical trick you missed. On a high nerd level I think multiplication is a cheaper operation than division.
exp(1) is used to generate the constant e that LUA doesn't provide. This was needed because we had some problems where CAPE was small and then the logarithm function goes to 0 for ln(1) and division by 0 is bad as well as for values below 1 it gives negative numbers that potentially turns the sign of the result around. Which by the logic of this macro turns CB into TCU. Probably it's not wise to have two things in one parameter and we should output CB and TCU as two separate fields but that's what it is now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just wondering why e(1), does it have some special significance. Why doesn't he just declare "const CAPElimit = 2.78".
I agree it's a bit weird that this parameter has different interpretation depending on the sign of the data value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I'll change it to that CAPElimit version then.
Following this script was quite easy. I also liked that I could see the whole logic inside for loop on my screen without having to scroll up and down. Often in the internet you see people arguing for "clean code" which seems to mean that everything should be split to as small functions as possible, which is maybe better for code reuse but it makes reading the code hard. |
I checked the code through and nothing obvious didn't pop out. I'm not that familiar with the code though. But the code and calculations seems to be well written and the code itself is very easy to understand. So merging is ok. |
Implementation of latest version of CB-TCU cloud. Smartmet Macro below:
https://wiki.fmi.fi/pages/viewpage.action?spaceKey=PROJEKTIT&title=Cb-TCu-top_FL