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

Mersenne Twister in in-line post #242

Closed
WenMeng-NOAA opened this issue Dec 16, 2020 · 17 comments · Fixed by #244
Closed

Mersenne Twister in in-line post #242

WenMeng-NOAA opened this issue Dec 16, 2020 · 17 comments · Fixed by #244
Assignees
Labels
bug Something isn't working

Comments

@WenMeng-NOAA
Copy link
Collaborator

Dusan found the mersenne twister module in UPP routine CALWXT_BOURG.f are not working properly:

[149] incalwxtbg, rn 3.3978019E+38 -3.3952875E+38
[145] in calwxtbg, jsta,jend= 33 64 im= 384
[145] in calwxtbg,me= 1 iseed= 302134645
[145] incalwxtbg, rn 3.3978019E+38 -3.3952875E+38

The expected rn range should be within in 0 to 1. The mersenne_twister in UPP is from NCEPLIBS w3em. This issue is cased different library precision used in ufs forecast component ( w3emc_d) and upp lib (w3emc_4) for in-line post.

@WenMeng-NOAA WenMeng-NOAA added the bug Something isn't working label Dec 16, 2020
@WenMeng-NOAA WenMeng-NOAA self-assigned this Dec 16, 2020
@WenMeng-NOAA
Copy link
Collaborator Author

WenMeng-NOAA commented Dec 17, 2020

Solution option 1)
Dusan proposed the fix via replace subroutine random_number (from mersenne_twister module) with the one from standard fortran function instead of w3emc module.
Dusan's proposal can be found at
DusanJovic-NOAA@b6aeef5

@WenMeng-NOAA
Copy link
Collaborator Author

Solution option 2)
Moorthi indicated he had committed the fix for this kind of issue in his feature branch:
SMoorthi-emc@b87c8e8

@WenMeng-NOAA
Copy link
Collaborator Author

Wen tested both option 1 and 2 with in-line post. Both option2 would fix rn value as expected range (1, 0).
With runtime logs, the expected changes would be like
from
incalwxtbg, rn 3.3978019E+38 -3.3952875E+38
into
incalwxtbg, rn 0.999998649116550 2.458319165258258E-005

@WenMeng-NOAA
Copy link
Collaborator Author

Huiya's comments:

I believe we switched UPP to use mersenne twister many years ago because the Fortran random number generator
had a bias. This is consistent with what Jun said that the former is a better random number generator.

We're hoping to replace precipitation type algorithms with GSL one so calwxt_* will be potentially retired in GFS v17.
With that said, I'm not sure how much time we want to spend on this.

@WenMeng-NOAA
Copy link
Collaborator Author

Wen created a new feature branch post_random_number and incorporate Moorthi's fixes at
WenMeng-NOAA@2327206

@climbfuji
Copy link
Contributor

My suggested approach for the mersenne twister is summarized in an issue for the ufs-weather-model: ufs-community/ufs-weather-model#324

@junwang-noaa
Copy link
Contributor

junwang-noaa commented Dec 17, 2020 via email

@WenMeng-NOAA
Copy link
Collaborator Author

@junwang-noaa Yes, the commented block in CALWXT_BOURG.f as you suggested from our last discussion. I will uncomment
that block for testing. There is related fixes in SURFACE.f. Do you have more comments on that part?

@junwang-noaa
Copy link
Contributor

junwang-noaa commented Dec 17, 2020 via email

@WenMeng-NOAA
Copy link
Collaborator Author

WenMeng-NOAA commented Dec 17, 2020 via email

@SMoorthi-emc
Copy link
Contributor

SMoorthi-emc commented Dec 17, 2020 via email

@WenMeng-NOAA
Copy link
Collaborator Author

Just commit WenMeng-NOAA@585c91d per Jun's last comment. A new interface for handle either real 4 or real 8. I am not familiar to interface structure in fortran. Please let me know your comments.

@junwang-noaa
Copy link
Contributor

junwang-noaa commented Dec 17, 2020 via email

@WenMeng-NOAA
Copy link
Collaborator Author

WenMeng-NOAA commented Dec 18, 2020 via email

@WenMeng-NOAA
Copy link
Collaborator Author

The solution comes to add a new UPP module upp_right_mersenne_twister which includes random_number routines for real 4 and real 8 and two types of routines switch via the interface inside of the module. This solution was tested with in-line post, off-line post on Dell, Cray, Hera and Orion.

@WenMeng-NOAA WenMeng-NOAA linked a pull request Dec 20, 2020 that will close this issue
@WenMeng-NOAA
Copy link
Collaborator Author

I spent a lot of time to tweak Moorth's fixes (keeping mersenne twister from w3emc) for working in in-line and off-line post tests last week. From Dusan's comments on PR #243, it seems that random_number call in new module upp_right_mersenne_twister is from Fortran intrinsic subroutine other than Mersenne twister from w3emc (I don't have enough fortran engine knowledge for that kind of modification).

Dusan's fixes would be straightforward for my UPP tests. One concern is switching to generic fortran routine random_number might cause some scientific issue. That's the reason mersenne_twister was adapted in UPP code many years ago. As Huiya's UPP project plan, we would adapt GSD precipitation type calculation method in GLOBAL model process in the future.

I would wait for comments from @HuiyaChuang-NOAA , @junwang-noaa and @SMoorthi-emc which fixes option we should adapt.

@HuiyaChuang-NOAA
Copy link
Collaborator

HuiyaChuang-NOAA commented Dec 21, 2020

I support Wen's solution to use Dusan's fix.

Please note that this fix will not be used in GFS V16 implementation and by the time RRFS is implemented, UPP will most likely already transition to GSL's explicit precipitation type algorithm. Once this fix is committed, it will support Public release.

Again, the subroutines calwxt* will become obsolete about a year from now. Wen already spent a lot of time on this. Let's use
Dusan's solution and move on.

Huiya

EricJames-NOAA pushed a commit to EricJames-NOAA/UPP that referenced this issue Dec 14, 2022
* Added support on Odin and Stampede

* Removed unnecessary module files
EricJames-NOAA pushed a commit to EricJames-NOAA/UPP that referenced this issue Dec 14, 2022
EricJames-NOAA pushed a commit to EricJames-NOAA/UPP that referenced this issue Dec 14, 2022
EricJames-NOAA added a commit to EricJames-NOAA/UPP that referenced this issue Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants