search

wrf-model / WRF

The official repository for the Weather Research and Forecasting (WRF) model
Other
1.26k stars 697 forks source link

Noah LSM bug for urban physics options #1676

Open sael9740 opened 2 years ago

sael9740 commented 2 years ago

I came across the following code which has been causing some issues for AceCAST (AceCAST is a GPU-accelerated version of WRF):

https://github.com/wrf-model/WRF/blob/f10d917d5c98473ea9b33e59f3ee29813454bd01/phys/module_sf_noahdrv.F#L1646-L1693

I won't get into the specifics of why this is a problem for AceCAST but I am relatively confident that there is a problem that you all should be aware of. Notice that emissi is never initialized inside the loop above but is used to calculate rl_up_rural on line 1693. This means it is probably using whatever value of emissi from the previous loops. Here are the assignments:

https://github.com/wrf-model/WRF/blob/f10d917d5c98473ea9b33e59f3ee29813454bd01/phys/module_sf_noahdrv.F#L814

https://github.com/wrf-model/WRF/blob/f10d917d5c98473ea9b33e59f3ee29813454bd01/phys/module_sf_noahdrv.F#L961

FYI - Just to be clear I found this issue due to how OpenACC was implicitly handling the variable emissi in my primary compute constructs for the lsm routine even when sf_urban_physics was turned off. Therefore, I do not have a test case demonstrating the issue but it looks pretty clear to me that there is one.

weiwangncar commented 2 years ago

@cenlinhe Can you take a look at this uninitialized problem? Should it be set to emiss(i,j) or something else?

cenlinhe commented 2 years ago

I do not see any bug for this EMISSI issue. EMISSI is an inout variable for SFLX subroutine from module_sf_noahlsm.F. For every I, J loop, the EMISSI is updated and output from the SFLX subroutine and further used in the above mentioned lines (1646-1694).

sael9740 commented 2 years ago

@cenlinhe - Yes EMISSI is passed to SFLX and modified for each iteration of the loops from lines 734-1567 but the problem is that it isn't initialized for each iteration in the loops from 1646-1740. The value of EMISSI for the latter set of loops will likely be whatever it was for the i=ite,j=jte iteration of the former set of loops. I couldn't possibly imagine this is the value that we want to use for ALL of the iterations of the latter set of loops.

cenlinhe commented 2 years ago

OK, now I see what you mean. Yes, it seems like a bug. I think the emissi in Line 1693 should be emiss_rural(i,j). @andreazonato Could you please confirm this?

sael9740 commented 2 years ago

I think the emissi in Line 1693 should be emiss_rural(i,j).

That would make more sense.

andreazonato commented 2 years ago

Those are not modifications I implemented. I'll forward it to Alberto Martilli asking for clarifications

andreazonato commented 2 years ago

OK, I spoke with Alberto. He said he discussed this term with Mike Barlage and he said it was the same. Anyway, Alberto is OK with this modification you suggest.

sael9740 commented 2 years ago

If you all don't mind I would appreciate your advice regarding these options. Let me know if this is not the proper channel for discussing such topics.

My goal internally is to ensure that AceCAST (GPU-WRF) generates the same results as the standard CPU version of WRF (within an acceptable error tolerance due to fp approximations, reductions, etc.). To do this I need to ensure that I am using test cases that properly exercise the code that I am working on. Is there a simple test case somewhere that I could use to test one or more of the sf_urban_physics options?