Fix line277 check icond.f90

[iamhappytoo]

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)
• [ ] tests passed
• [ ] new tests added
• [ ] science test figures
• [ ] checked that the new code conforms to the SUMMA coding conventions
• [ ] ReleaseNotes entry

[iamhappytoo]

changed part "(a,1x,i0)" to "(a,1x,i0,a,f5.3,a,f5.3)" in check_icond.f90 line 277 to print out error correctly.

[wknoben]

Thanks for the PR! Could you please address the following small points?

1. Could you add a brief description (in a comment here) of what happens before your fix, and what happens after?
2. Could you clean up the commits, so that the only commit listed is the one the actually changed the file? The one that says "merge pull request 1 from [...]" should not be in the list.
3. Could you add a brief mention of your fix to the Release Notes document? You can find this file as part of your SUMMA repo, under ./summa/docs/whats-new.md

[iamhappytoo]

Before the fix, summa initial condition check (check_icond.f90) error print format does not comply with the transferred input, failed to print out the error message, with the warning: "Fortran runtime error: Expected INTEGER for item 4 in formatted transfer, got REAL" After the fix, summa can successfully print out the error message.

[iamhappytoo]

Hi Wouter,

Thanks for the helpful feedback! In response yout comment #2, I created a new pull request #504 with a clean commit (two changes, one to the check_icond.f90, one to the whats-new.md (in response to comment #3)), and added the brief description (in response to comment #1) to the commit history.

Cheers, Zhaoxin

On Tue, Mar 1, 2022 at 9:46 AM Wouter Knoben @.***> wrote:

Thanks for the PR! Could you please address the following small points?

1. Could you add a brief description (in a comment here) of what happens before your fix, and what happens after?
2. Could you clean up the commits, so that the only commit listed is the one the actually changed the file? The one that says "merge pull request 1 from [...]" should not be in the list.
3. Could you add a brief mention of your fix to the Release Notes document? You can find this file as part of your SUMMA repo, under ./summa/docs/whats-new.md

— Reply to this email directly, view it on GitHub https://github.com/CH-Earth/summa/pull/503#issuecomment-1055696270, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHYNHWXPEKSV2J6ENKSQUH3U5ZJWPANCNFSM5PC4LBCA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

-- Zhaoxin Ban Ph.D. Candidate Land Surface Hydrology Research Group http://hydro.ucla.edu/ University of California, Los Angeles Phone (O): +1 (424)385-4256

[andywood]

One comment -- perhaps we should develop some guidance on what should be added to the 'what's new' & release notes documents. If a PR is for fixing a print statement, I'd argue that we don't update 'what's new' and save that for notable upgrades that expand or improve the functionality of SUMMA, or implement a major bug fix. For smaller bug fixes and adjustments (which are routine and expected), we may want to start a 'bugfix' document. Note, I'm not disparaging this PR -- all these efforts to correct and develop the SUMMA code are valuable and appreciated.

On Wed, Mar 2, 2022 at 3:17 PM ZhaoxinBan @.***> wrote:

Hi Wouter,

Thanks for the helpful feedback! In response yout comment #2, I created a new pull request #504 with a clean commit (two changes, one to the check_icond.f90, one to the whats-new.md (in response to comment #3)), and added the brief description (in response to comment #1) to the commit history.

Cheers, Zhaoxin

On Tue, Mar 1, 2022 at 9:46 AM Wouter Knoben @.***> wrote:

Thanks for the PR! Could you please address the following small points?

1. Could you add a brief description (in a comment here) of what happens before your fix, and what happens after?
2. Could you clean up the commits, so that the only commit listed is the one the actually changed the file? The one that says "merge pull request 1 from [...]" should not be in the list.
3. Could you add a brief mention of your fix to the Release Notes document? You can find this file as part of your SUMMA repo, under ./summa/docs/whats-new.md

— Reply to this email directly, view it on GitHub https://github.com/CH-Earth/summa/pull/503#issuecomment-1055696270, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AHYNHWXPEKSV2J6ENKSQUH3U5ZJWPANCNFSM5PC4LBCA

. Triage notifications on the go with GitHub Mobile for iOS < https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675

or Android < https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub .

You are receiving this because you authored the thread.Message ID: @.***>

-- Zhaoxin Ban Ph.D. Candidate Land Surface Hydrology Research Group http://hydro.ucla.edu/ University of California, Los Angeles Phone (O): +1 (424)385-4256

— Reply to this email directly, view it on GitHub https://github.com/CH-Earth/summa/pull/503#issuecomment-1057450708, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIKARINLBW2VTELWTIS6PTU57SGNANCNFSM5PC4LBCA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

[wknoben]

@andywood I agree that it may make sense to separate larger and smaller changes, but I'm currently coming up blank when it comes to a way to distinguish between the two. It seems mostly intuitive to me whether a change is one or the other.

I think at the very least we can update the PR template to be a bit more explicit about how to add a ReleaseNotes entry (I'll do that in a moment). Perhaps that way we can at least track all changes without a lot of hassle and sort them into major/minor when we do a new release?

[andywood]

@wouter, I agree that it's intuitive/subjective. I'd suggest having a release notes and a bugfix/minor change document, and let people choose which one to add their change to -- then when there's a release, they could be sorted out before the release but it might make it easier. There could be come guidelines in the documents themselves -- ie something that changes the functionality of code, adds a feature, changes default settings, changes code convention (ie a major variable renaming) might be worth putting in release-notes. Something a user should know about and why it happened. Alternatively things like correcting misspellings, formatting, adding a minor initialization, fixing some edge case that rarely occurs, adding a print statement somewhere ... probably goes more in the bugfix/minor category.

On Mon, Mar 14, 2022 at 12:57 PM Wouter Knoben @.***> wrote:

@andywood https://github.com/andywood I agree that it may make sense to separate larger and smaller changes, but I'm currently coming up blank when it comes to a way to distinguish between the two. It seems mostly intuitive to me whether a change is one or the other.

I think at the very least we can update the PR template to be a bit more explicit about how to add a ReleaseNotes entry (I'll do that in a moment). Perhaps that way we can at least track all changes without a lot of hassle and sort them into major/minor when we do a new release?

— Reply to this email directly, view it on GitHub https://github.com/CH-Earth/summa/pull/503#issuecomment-1067176187, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIKARN2RG5DSO2HSWV2LPLU76D2VANCNFSM5PC4LBCA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

[wknoben]

@andywood I see. I opened a new PR along these lines #505 Could you have a look to see if this is what you had in mind?

[wknoben]

Closing this now because we have a new PR for the original issue, and the resulting discussion about tracking changes the code in a document has been resolved in #505