search

NCAR / wrf_hydro_nwm_public

WRF-Hydro model code
https://ral.ucar.edu/projects/wrf_hydro
Other
181 stars 139 forks source link

Update reach / lake IDs to 64-bit #582

Closed rcabell closed 3 years ago

rcabell commented 3 years ago

TYPE: enchantment

KEYWORDS: nhd,reach,hydrofabric

SOURCE: Ryan@NCAR

DESCRIPTION OF CHANGES: In order to support the new NHDPlus High Resolution dataset that uses large-integer IDs, the routing and lake code required updates to use 64-bit integers internally.

TESTS CONDUCTED: Extensive testing with Alaska NWM domain

Checklist

Merging the PR depends on following checklist being completed. Add X between each of the square brackets if they are completed in the PR itself. If a bullet is not relevant to you, please comment on why below the bullet.

scrasmussen commented 3 years ago

It looks like the MPI_INTEGER to MPI_INTEGER8 changes fixed two of the four failing CI tests. Will investigate why the others are failing.

scrasmussen commented 3 years ago

I looked into the failing CI tests and they seems to all fail at this line and this line.

The failing tests are due to this type errors:

DIFFER : VARIABLE : feature_id : TYPE : INT <> INT64

Happily, this is to be expected due to the fact the the integer size has been changed.

I ran the CI tests with the following command. I tested with thereach, gridded, nwm_ana, nwm_long_range configs.

$ python3 run_tests.py --config gridded --compiler gfort --output_dir /glade/work/soren/OUTPUT --candidate_dir /glade/u/home/soren/src/wrf_hydro/64bit --reference_dir /glade/u/home/soren/src/wrf_hydro/upstream --domain_dir /glade/p/cisl/nwc/model_testing_domains/croton_NY --ncores 4 --xrcmp_n_cores 0 --print --pdb

When the tests would fail it would enter the debugger and I'd save that output and then continue. I've attached the file with the failed outputs to confirm the errors were from the INT64 error. Note, I first ran these with one core, whose output is shown in the file, and then reran with four and got the same results.

ci_tests_output09242021.md

rcabell commented 3 years ago

CI tests now show only DIFFER : VARIABLE : feature_id : TYPE : INT <> INT64 errors while all other components pass, so I think we're good to go. If @scrasmussen and @aubreyd approve the PR, I'll merge it.

aubreyd commented 3 years ago

Confirmed the Alaska domain with 64-bit int IDs is producing identical answers to the previous run over a 10-day cold start simulation. Looks good from my side!

thwllms commented 3 years ago

Sorry to insert myself into the conversation here, but does this mean that the next version of the National Water Model will be based on NHDPlus High Resolution instead of NHDPlusV2? Basically wondering if the IDs in the NetCDF files posted here and to Google Cloud Storage / S3 will be changing.

aubreyd commented 3 years ago

The new implementation of the NWM in South-Central Alaska will be using NHD High-Resolution products. The CONUS implementation will stay as-is.