CH-Earth / summa

Structure for Unifying Multiple Modeling Alternatives:
http://www.ral.ucar.edu/projects/summa
GNU General Public License v3.0
80 stars 106 forks source link

Update modelwrite.f90 to add in hruId to restart files #575

Open DaveCasson opened 1 month ago

DaveCasson commented 1 month ago

The need for the hruId index has been discussed in several issues. Notably https://github.com/CH-Earth/summa/issues/253 Also https://github.com/CH-Earth/summa/pull/419, https://github.com/CH-Earth/summa/issues/401

This PR adds writing of hruId to the restart file without modification to other code and while using the existing hruInfo structure.

Make sure all the relevant boxes are checked (and only check the box if you actually completed the step):

wknoben commented 1 month ago

Nice, thanks! Couple of things for posterity:

DaveCasson commented 1 month ago

1) GRUs are not currently used at all in the state file, but in testing it appears that there is rationale that GRUs (and gruIds) may need to be included. That is, you can merge currently a number of distributed state files, but there order of the hruIds is not guaranteed to match that the attributes (for example). I'm not sure if adding grus would alleviate this issue or not. It is left as the responsibility of the modeller (and a difficult mistake to track without hruIds).

2) I don't foresee a case this would break existing code, as it is an additional variable only. Adding GRUs could add more risk? I do not know.

3)The hru ids being assigned in the correct order comes down to this loop:

! Allocate and fill hruIds array allocate(hruIds(nHRU)) do iGRU = 1, nGRU do iHRU = 1, gru_struc(iGRU)%hruCount cHRU = gru_struc(iGRU)%hruInfo(iHRU)%hru_ix hruIds(cHRU) = int(gru_struc(iGRU)%hruInfo(iHRU)%hru_id, kind=i4b) end do end do

So it is a direct pull and write from that existing data structure. A unit test to compare say to attribute hruId could be useful, but support would be needed to see if / why / how that would be implemented.