Update var_lookup.f90 to add wallClockTime.

CH-Earth / summa

Structure for Unifying Multiple Modeling Alternatives:

http://www.ral.ucar.edu/projects/summa

GNU General Public License v3.0

80 stars 105 forks source link

Update var_lookup.f90 to add wallClockTime. #519

Closed ZacharyWills closed 1 year ago

ZacharyWills commented 1 year ago

To correct an issue where running SUMMA without this correction fails due to fatal error.

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

[ ] Closes #xxx (identify the issue associated with this PR)
[ ] 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)
[x] Checked that the new code conforms to the SUMMA coding conventions
[ ] Describe the change in the release notes (use either ./summa/docs/whats-new.md or ./summa/docs/minor-changes.md depending on what changed)

ZacharyWills commented 1 year ago

I can open an issue describing the problem, but I just built the 3.1.2 version of SUMMA using the Dockerfile in the root of the repo.

wknoben commented 1 year ago

Hi @ZacharyWills , Thanks for the contribution. Did you build the SUMMA exe from the master branch by any chance? PR #465 (closed, merged into develop) contains all the necessary infrastructure to use the wallClockTime variable.

If you did build the exe from master, it would be helpful if you can open an issue that explains how you did that. As far as I can tell you shouldn't end up needing wallClockTime infra if building from master.

If you did build from develop, an issue would be helpful too because that should just work.

I'm a little busy today but happy to give a hand figuring this out.

ZacharyWills commented 1 year ago

I'll rebuild from both (master is what I used when I got the error) and post a command history of both builds.

ZacharyWills commented 1 year ago

Also if you want to pull down the ENV, it's here: https://hub.docker.com/r/zwills/summa3-1-2

ZacharyWills commented 1 year ago

So I built both the master branch and the develop branch again, and the develop branch did work.

At this point it looks like the develop branch rolls in the fix, but I'm wondering if the master could merge this before your next large merge so that the master branch works in the interim. (this depends on how long it is until the next merge, which might render this a non-issue) Thanks for your help!

Also here are the images if you want to try yourself:

wknoben commented 1 year ago

Thanks for checking. Would you be willing to run one more (hopefully final) test?

I'm speculating that the executable from master breaks because your setup is asking it to provide an output that it doesn't know about (because the PR that adds variable wallClockTime hasn't yet been merged into master). Could you try to remove the wallClockTime line from your outputControl.wb.txt file and re-run the master exe?

If this also solves the problem we can postpone prematurely merging develop into master, and we'll have a record of what we discussed and found in case others run into this.

wknoben commented 1 year ago

Hi @ZacharyWills , just checking in - have you got everything you need to proceed?

On our side I've tried to see if we have any example outputControl.txt files out there that specify the wallClockTime variable without being specific about this needing code from the dev branch, but I've not been able to find any such files. It would help us ensure consistency between the code base and any examples if you could let us know where you got your outputControl.txt file from.

wknoben commented 1 year ago

Given how this seems solved I'll close it for now. Feel free to re-open an issue if you run into anything else.