Skip to content
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

initial commit of meps updraft helicity #13

Merged
merged 2 commits into from
Jul 7, 2023
Merged

initial commit of meps updraft helicity #13

merged 2 commits into from
Jul 7, 2023

Conversation

tackandr
Copy link
Contributor

Code review for lua script related to updraft helicity. This is somewhat unconventional so I wanted to have some review on it. It's on the edge of the lua limitations but I wanted to give it a try this way first as it is quick to write but if needed I could do it as a full plugin as well.

@tackandr tackandr requested a review from mpartio June 27, 2023 08:53
@tackandr tackandr self-assigned this Jun 27, 2023
@mpartio
Copy link
Member

mpartio commented Jun 28, 2023

I think I got the basic idea but the actual definition of updraft helicity is still a bit of a mystery to me. Could you provide some documentation for example a link to a page where UH is explained?

Some variables are not set, like dx and mode. Also you have hard coded grid properties in the file but I guess those are only for testing.

The code itself is quite easy to follow. How is the performance? I would guess running a luatool over all hybrid levels isn't exactly fast? If the performance is sufficient, I think this lua script is nice and short. If the performance is bad, maybe some could could be implemented in c++ and/or cuda?

@tackandr
Copy link
Contributor Author

I think this paper describes the idea in detail
https://www.spc.noaa.gov/publications/milne/vert-uh.pdf

Missing variables got eaten by the copycat. But yes, probably need to be read from the configuration in the production version.

Regarding performance:
Configuration that precomputes hybrid levels 21-40 and then calls the hitool took 3500ms on pansuola for a single time step and about 2min for a 67 step forecast.

@mpartio
Copy link
Member

mpartio commented Jun 29, 2023

Thanks, I think I got the picture what UH is about. I think this Lua script is good because even I could understand it 😄

With those timings I don't think performance will be an issue. Production machines are probably also a bit faster than pansuola. And if it turns out to be too slow, some parts can be moved to c++ (or the whole code).

👍

@tackandr tackandr merged commit 548ef94 into master Jul 7, 2023
@tackandr tackandr deleted the STU-21760 branch October 27, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants