Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

DaveCasson
Copy link

The need for the hruId index has been discussed in several issues. Notably #253 Also #419, #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):

  • [ x] Closes Error when using restart files #253
  • Code passes standard test cases (results are either bit-for-bit identical, or differences are explained in the PR comment)
  • New tests added (describe which tests were performed to test the changes)
  • Science test figures (add figures to PR comment and describe the tests)
  • Checked that the new code conforms to the SUMMA coding conventions
  • Describe the change in the release notes (use ./summa/docs/whats-new.md)

The need for the hruId index has been discussed in several issues. Notably CH-Earth#253
Also CH-Earth#419, CH-Earth#401

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

wknoben commented Oct 9, 2024

Nice, thanks! Couple of things for posterity:

  • Is there a reason why we cannot easily keep GRU IDs with the same approach?
  • Will this break any existing code that relies on restart files?
  • Do we need any tests to confirm the the HRU IDs come out in the correct order?

@DaveCasson
Copy link
Author

  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.

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.

2 participants