Skip to content

Conversation

@NickSzapiro-NOAA
Copy link

This fixes the compiler warning:

mersenne_twister.F90:256:38:
            get(n+3:nrest)=transfer(stat%gset,get,nrest-(n+3)+1)
Warning: Intrinsic TRANSFER has partly undefined result: source size 8 < result size 16 [-Wsurprising]

and was bit-for-bit in ufs-weather-model regression tests a couple weeks ago.

The solution is by @DusanJovic-NOAA, with this context:

gset is declared as :
$ grep ':: gset' stochastic_physics/mersenne_twister.F90 FV3/ccpp/physics/physics/Radiation/mersenne_twister.f
stochastic_physics/mersenne_twister.F90:          real(kind_dbl_prec):: gset
FV3/ccpp/physics/physics/Radiation/mersenne_twister.f:          real(kind_dbl_prec):: gset
kind_dbl_prec is 8 bytes (64-bits) so get array (integer) should also be 64-bits or 2 element 32-bits array, 
to match the bitwise length of the gset. 

So to be 2-element array, the
get(n+3:nrest) =nrest 
must be n+4, and it is now defined as:
integer,parameter:: nrest=n+6

The caveat is that we are not familiar with this code, so we're hoping somebody who really understands what's going on here should double check this

The need is that, currently, ufs-weather-model RTs show a pattern of a "ball of diffs" in cpld_debug_gefs_intel (and cpld_debug_gefsv13_intel), failing in run-to-run reproducibility (ufs-community/ufs-weather-model#2675).

A test changing this in both stochastic physics and ccpp improves reproducibility in these tests. The matching stochastic physics PR is at NOAA-PSL/stochastic_physics#85

@BryanFlynt
Copy link

Looking this over I believe the new implementation with nrest=n+4 is the correct way to go.
Using a value of n+6 does not align because your trying to copy two 32-bit integers between one 64-bit real on these lines.

sstat%gset=transfer(put(n+3:nrest),sstat%gset)

and

get(n+3:nrest)=transfer(sstat%gset,get,nrest-(n+3)+1)

A person could simplify the real64 to int32 copy by allowing the transfer() function to determine the size.

get(n+3:nrest)=transfer(sstat%gset,get)  ! gset is size of 2 get's

FWIW,
I get concerned any time complex bit manipulations are being done this way yet the types are not explicitly defined (i.e. int32, int64, real32, real64). The documentation in the header says all integer must be 32-bit so why not explicitly declare them as such and then inadvertent compiler flags (-fdefault-integer-8) won't break things.

Copy link
Collaborator

@grantfirl grantfirl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm definitely not an expert in the mersenne twister code, but it's hard to argue with the change. This is a prime candidate to combine with other PRs if you're OK with it because it is small and (according to your tests) doesn't change RT baselines.

@NickSzapiro-NOAA
Copy link
Author

Thanks @BryanFlynt @grantfirl . Maybe let me confirm next week that tests are bit-for-bit on several platforms, and then I'm happy for this to be combined elsewhere.

@NickSzapiro-NOAA
Copy link
Author

ufs-weather-model RTs are bit-for-bit on hera, derecho, and wcoss2. afaik, this is ok to combine with other PRs

@grantfirl
Copy link
Collaborator

Combined into #295. Keeping open until merged.

@rhaesung rhaesung merged commit 59b9bc1 into ufs-community:ufs/dev Jul 11, 2025
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.

6 participants