NCAR / noahmp

Noah-MP Community Repository
Other
71 stars 81 forks source link

Index out of bound for Urban LCZ cases in conversion of lfmass #104

Closed bfkg closed 10 months ago

bfkg commented 11 months ago

If urban with LCZ is selected IVGTYP ranges from 1-61. SLA_TABLE is however only defined up to 20/27 depending on the landuse dataset selected. This leads to reading beyond the bounds of SLA_TABLE.

             masslai = 1000. / max(SLA_TABLE(IVGTYP(I,J)),1.0)

https://github.com/NCAR/noahmp/blob/981d4f859ce6c64213d38a783654c05b47b3485e/drivers/wrf/module_sf_noahmpdrv.F#L2184C14-L2184C101

Maybe something like this can solve the issue:


if(urbanpt_flag)  then
   masslai = 1000. / max(SLA_TABLE(NATURAL_TABLE),1.0)
else 
   masslai = 1000. / max(SLA_TABLE(IVGTYP(I,J)),1.0)
endif
cenlinhe commented 11 months ago

thank you for pointing out this issue. I believe we already fixed this issue a few months ago and it is now in our updated master and develop branches: https://github.com/NCAR/noahmp/blob/master/drivers/hrldas/NoahmpInitMainMod.F90#L190-L196

bfkg commented 11 months ago

Will the WRF driver also be updated for the next WRF release?

cenlinhe commented 11 months ago

I am not sure why WRF seems doing OK with this issue even without this fix. @tslin2 Have you run into any problems using WRF-urban-Noah-MP with LCZ for this issue: https://github.com/NCAR/noahmp/blob/981d4f859ce6c64213d38a783654c05b47b3485e/drivers/wrf/module_sf_noahmpdrv.F#L2184

It seems that we also need to fix it in the non-refactored WRF/NoahMP code using the same above fix that has been included in the refactor code.

tslin2 commented 11 months ago

Hi, @cenlinhe and @bfkg

I don't encounter any issue in WRF-Noah-MP-urban with LCZ. I made a quick test. SLA_TABLE(IVGTYP(I,J)) was zero , so masslai was 1000.0 using LCZ

tslin2 commented 11 months ago

Hi @cenlinhe,

https://github.com/NCAR/noahmp/blob/6c9717b6710f30ff7217ebc51cd93e2fa578d516/drivers/hrldas/NoahmpInitMainMod.F90#L190-L196

Look like this fix should use urbanpt_flag as @bfkg suggested for the if statement because there could be not urban types such as in rural grids (like forest, or grass or others) with WRF-urban model turn on (NoahmpIO%SF_URBAN_PHYSICS /= 0).

I am also thinking the NATURAL_TABLE should change to ISURBAN_TABLE, so the result can be the same as the original single urban type when LCZ is not used but WRF-urban model turn on.

if ( urbanpt_flag ) then NoahmpIO%LFMASSXY(I,J) = NoahmpIO%LAI(I,J) 1000.0 / & max(NoahmpIO%SLA_TABLE(NoahmpIO%ISURBAN_TABLE),1.0)! use LAI to initialize (v3.7) else NoahmpIO%LFMASSXY(I,J) = NoahmpIO%LAI(I,J) 1000.0 / & max(NoahmpIO%SLA_TABLE(NoahmpIO%IVGTYP(I,J)),1.0) ! use LAI to initialize (v3.7) endif

cenlinhe commented 11 months ago

@tslin2 I agree with you that we should use urbanpt_flag, but I do not think we should use "ISURBAN_TABLE" because the idea here is that if urban physics is turned on and Noah-MP column model here will simulate the rural portion of the urban grid. We should not use the bulk urban type "ISURBAN_TABLE" for the SLA parameter.

cenlinhe commented 11 months ago

I will try to fix this issue in both HRLDAS and WRF versions this week or next week.

tslin2 commented 11 months ago

got it, thanks!

cenlinhe commented 10 months ago

I have fixed this bug in Noah-MP v4.5 at this commit: https://github.com/NCAR/noahmp/commit/6f2bf7c575a0099d60b51efe8811c6519d531abe I have also submitted a WRF PR to include this fix: https://github.com/wrf-model/WRF/pull/1971

cenlinhe commented 10 months ago

I am going to close this issue since it has been solved.

tslin2 commented 10 months ago

should this also be fixed in the version 5.0?!

cenlinhe commented 10 months ago

I have fixed this in the version 5.0 too at this commit: https://github.com/NCAR/noahmp/commit/843a742c6019a613450f0002e7ec65f44e49b523