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.

Sign up for GitHub

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

Jump to bottom

Further modifications to check_energy.F90 #1498

Merged
merged 3 commits into from
Jun 6, 2017
Merged

Further modifications to check_energy.F90 #1498

merged 3 commits into from
Jun 6, 2017

Conversation

kaizhangpnl
Copy link
Contributor

@kaizhangpnl kaizhangpnl commented May 5, 2017 •
edited
Loading

Further modifications to check_energy.F90 to address the need from the coupled team. Cleaned up the IEFLX code and added more comments. Please note that the namelist variable l_ieflx_fix was removed and the fixer can be switched on if ieflx_opt is 1 or 2 (recommended value is 2).

The problem associated with #1479 went away with those changes.

SHFLXFIX was removed. Instead, we output SHFLXORI (SHFLX before applying the IEFLX fixer).

IEFLX is now a constant field (global mean IEFLX) and default output.

Also removed the pbuf variable for IEFLX, since it is not used at this time.

[BFB] for prognostic fields, but IEFLX and SHFLXFIX are changed/removed.

Fixes #1479

Zhang added 2 commits May 5, 2017 09:31
Bugfix for the IEFLX fixer
10e171f 
Fixed a bug in check_energy.F90, which will cause runtime error if
subcol treatment is used.

Also cleaned up the IEFLX code and added more comments

[BFB]
minor comment
a27a1ba
@kaizhangpnl kaizhangpnl added bug fix PR Atmosphere BFB PR leaves answers BFB labels May 5, 2017
@kaizhangpnl
Copy link
Contributor Author

Hi @susburrows I assigned this bug fix PR to you, since you helped to integrate the original PR (#1303). Feel free to re-assign if needed. Thanks!

@E3SM-Project E3SM-Project locked and limited conversation to collaborators May 10, 2017
@rljacob
Copy link
Member

rljacob commented May 19, 2017

@singhbalwinder go ahead and start merging this.

@singhbalwinder
Copy link
Contributor

I talked with @kaizhangpnl few days back and he told me to hold on until he resolves some issues regarding this PR.

@kaizhangpnl : Is it ready to go now?

@kaizhangpnl
Copy link
Contributor Author

Please hold on for one more day. I am making a couple of further changes to address the need from the coupled team.

@kaizhangpnl
Further modifications to check_energy.F90 to address the need
920864c 
from the coupled team.

SHFLXFIX was removed. Instead, we output SHFLXORI (SHFLX before
applying the IEFLX fixer).

IEFLX is now a constant field (global mean IEFLX).

Also removed the pbuf variable for IEFLX, since it is not used
at this time.

[BFB] for prognostic fields, but IEFLX and SHFLXFIX are changed/removed.
@kaizhangpnl kaizhangpnl requested review from golaz and tangq May 19, 2017 23:43
@kaizhangpnl
Copy link
Contributor Author

kaizhangpnl commented May 19, 2017 •
edited
Loading

I just committed the new changes and asked @golaz and @tangq to see whether there is anything else to be improved.

@E3SM-Project E3SM-Project unlocked this conversation May 19, 2017
@tangq
Copy link
Contributor

tangq commented May 19, 2017

Looks good me. I like the comments regarding different IEFLX options. Good job.

@kaizhangpnl
Copy link
Contributor Author

Thanks for the quick review.

@rljacob
Copy link
Member

rljacob commented May 23, 2017

Does this fix #1479?

@kaizhangpnl
Copy link
Contributor Author

Yes, the related code was removed.

@rljacob
Copy link
Member

rljacob commented May 23, 2017

Ok. Please update the PR description then.

@rljacob
Copy link
Member

rljacob commented May 23, 2017

You should add a github keyword on a single line at the end of the description to close the issue like this:
Fixes #1479

See the merge commit message example in https://acme-climate.atlassian.net/wiki/display/Docs/Commit+message+template

@kaizhangpnl
Copy link
Contributor Author

Thanks, but the guide says that's for integrator to merge commit messages?

@kaizhangpnl kaizhangpnl changed the title Bugfix for the IEFLX fixer Further modifications to check_energy.F90 May 23, 2017
@rljacob
Copy link
Member

rljacob commented May 23, 2017 •
edited
Loading

Yes the integrator will make the merge commit message but they just copy and paste what's in the PR description at the top. I realize now that the docs aren't very clear on this.

@kaizhangpnl
Copy link
Contributor Author

I see, thanks.

@kaizhangpnl
Copy link
Contributor Author

@singhbalwinder Since there is no more new comment, I think it's ready to be merged. Thanks.

@singhbalwinder
Copy link
Contributor

@rljacob: I am not sure if there is a rule that reviewers have to officially approve (using github review process) before a branch can be merged. @kaizhangpnl has indicated that this is ready to merge. Should I go ahead and merge it?

@rljacob
Copy link
Member

rljacob commented May 30, 2017

Yeah we haven't spelled that out. @tangq said it was ok in a comment. If @kaizhangpnl doesn't want the other requested review, then its ok to merge.

@rljacob
Copy link
Member

rljacob commented May 30, 2017

If no one has officially requested changes with a review, its up to the integrator and the developer.

@kaizhangpnl kaizhangpnl removed the request for review from golaz May 30, 2017 23:39
@kaizhangpnl
Copy link
Contributor Author

Hi, I just made a change. I think Qi can represent the coupled team. :)

@singhbalwinder
Copy link
Contributor

Thanks @rljacob and @kaizhangpnl . I will wait for @tangq review before proceeding.

@kaizhangpnl
Copy link
Contributor Author

Hi @singhbalwinder Qi already reviewed the code. See above.

@singhbalwinder
Copy link
Contributor

Ok. Thanks @kaizhangpnl. I will merge this to next when it is open for merging.

jgfouca pushed a commit that referenced this pull request Jun 2, 2017
@fischer-ncar
Add new grid aliases for gx3v7 and usgs masks. This is being done to
c36bf99 
match the new grid alias style that includes the masks, mg16, mg17,
mg37, mgusgs.

Test suite: scripts_regression_test
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes #1498

User interface changes?:

Code review:
@singhbalwinder
Copy link
Contributor

@kaizhangpnl : Does this PR change/remove/add any variable in the history files or any variables which are sent to the coupler? If yes, we should probably list this PR as Non-BFB?

@kaizhangpnl
Copy link
Contributor Author

@singhbalwinder As written in the commit message, the solution is BFB, but IEFLX is added as default and SHFLXFIX removed. No modifications to the coupler. If the testing simulations will output and compare h0 files with those output from the previous model, it would be different.

@kaizhangpnl
Copy link
Contributor Author

@jgfouca the commit c36bf99 says it fixes this pull request. Is it a typo?

@rljacob
Copy link
Member

rljacob commented Jun 2, 2017

When fields are added/removed in a history file, I don't think the cprnc test will FAIL. It only fails if the content of a specific field with the same name is different.

@rljacob
Copy link
Member

rljacob commented Jun 2, 2017

This "Fixes" in c36bf99 is in reference to an issue with that number in ESMCI. This is an unfortunate side effect of using subtree for CIME and not referencing issues with the complete repo path.

@kaizhangpnl
Copy link
Contributor Author

Thanks for the information. @singhbalwinder please let me know if you see any strange failed test. thanks!

@singhbalwinder
Copy link
Contributor

Thanks @kaizhangpnl and @rljacob . I didn't know that CPRNC only fails when the fields are different. I will merge it to next soon.

singhbalwinder added a commit that referenced this pull request Jun 5, 2017
@singhbalwinder
Merge branch 'kaizhangpnl/atm/ieflx_cleanup' into next (PR #1498)
961c4c8 
Further modifications to check_energy.F90 to address the need from the
coupled team. Cleaned up the IEFLX code and added more comments.
Please note that the namelist variable l_ieflx_fix was removed and the
fixer can be switched on if ieflx_opt is 1 or 2 (recommended value is
2).

The problem associated with #1479 went away with those changes.

SHFLXFIX was removed. Instead, we output SHFLXORI (SHFLX before
applying the IEFLX fixer).

IEFLX is now a constant field (global mean IEFLX) and default output.

Also removed the pbuf variable for IEFLX, since it is not used at this
time.

[BFB] for prognostic fields, but IEFLX and SHFLXFIX are changed/removed.

Fixes #1479

* kaizhangpnl/atm/ieflx_cleanup:
  Further modifications to check_energy.F90 to address the need from the
coupled team.
  minor comment
  Bugfix for the IEFLX fixer

Conflicts:
	components/cam/src/physics/cam/phys_control.F90
@singhbalwinder
Copy link
Contributor

pushed to next.

@kaizhangpnl
Copy link
Contributor Author

Thanks! Fingers crossed. :)

@singhbalwinder
Copy link
Contributor

Everything went well with the nightly testing so I am merging it to master.

@singhbalwinder singhbalwinder merged commit 920864c into master Jun 6, 2017
singhbalwinder added a commit that referenced this pull request Jun 6, 2017
@singhbalwinder
Merge branch 'kaizhangpnl/atm/ieflx_cleanup' (PR #1498)
8abf0b8 
Further modifications to check_energy.F90

Further modifications to check_energy.F90 to address the need from the
coupled team. Cleaned up the IEFLX code and added more comments.
Please note that the namelist variable l_ieflx_fix was removed and the
fixer can be switched on if ieflx_opt is 1 or 2 (recommended value is
2).

The problem associated with #1479 went away with those changes.

SHFLXFIX was removed. Instead, we output SHFLXORI (SHFLX before
applying the IEFLX fixer).

IEFLX is now a constant field (global mean IEFLX) and default output.

Also removed the pbuf variable for IEFLX, since it is not used at this
time.

[BFB] for prognostic fields, but IEFLX and SHFLXFIX are changed/removed.

Fixes #1479

* kaizhangpnl/atm/ieflx_cleanup:
  Further modifications to check_energy.F90 to address the need from the coupled team.
  minor comment
  Bugfix for the IEFLX fixer

Conflicts:
	components/cam/src/physics/cam/phys_control.F90
@kaizhangpnl
Copy link
Contributor Author

Great. Thanks!

@kaizhangpnl kaizhangpnl deleted the kaizhangpnl/atm/ieflx_cleanup branch July 25, 2017 17:15
jgfouca pushed a commit that referenced this pull request Oct 25, 2017
@singhbalwinder
Merge branch 'kaizhangpnl/atm/ieflx_cleanup' (PR #1498)
5e2d656 
Further modifications to check_energy.F90

Further modifications to check_energy.F90 to address the need from the
coupled team. Cleaned up the IEFLX code and added more comments.
Please note that the namelist variable l_ieflx_fix was removed and the
fixer can be switched on if ieflx_opt is 1 or 2 (recommended value is
2).

The problem associated with #1479 went away with those changes.

SHFLXFIX was removed. Instead, we output SHFLXORI (SHFLX before
applying the IEFLX fixer).

IEFLX is now a constant field (global mean IEFLX) and default output.

Also removed the pbuf variable for IEFLX, since it is not used at this
time.

[BFB] for prognostic fields, but IEFLX and SHFLXFIX are changed/removed.

Fixes #1479

* kaizhangpnl/atm/ieflx_cleanup:
  Further modifications to check_energy.F90 to address the need from the coupled team.
  minor comment
  Bugfix for the IEFLX fixer

Conflicts:
	components/cam/src/physics/cam/phys_control.F90
@rljacob rljacob mentioned this pull request Feb 5, 2019
Merged
@kaizhangpnl kaizhangpnl mentioned this pull request Feb 6, 2019
Closed
rljacob pushed a commit that referenced this pull request May 6, 2021
@singhbalwinder
Merge branch 'kaizhangpnl/atm/ieflx_cleanup' (PR #1498)
04af460 
Further modifications to check_energy.F90

Further modifications to check_energy.F90 to address the need from the
coupled team. Cleaned up the IEFLX code and added more comments.
Please note that the namelist variable l_ieflx_fix was removed and the
fixer can be switched on if ieflx_opt is 1 or 2 (recommended value is
2).

The problem associated with #1479 went away with those changes.

SHFLXFIX was removed. Instead, we output SHFLXORI (SHFLX before
applying the IEFLX fixer).

IEFLX is now a constant field (global mean IEFLX) and default output.

Also removed the pbuf variable for IEFLX, since it is not used at this
time.

[BFB] for prognostic fields, but IEFLX and SHFLXFIX are changed/removed.

Fixes #1479

* kaizhangpnl/atm/ieflx_cleanup:
  Further modifications to check_energy.F90 to address the need from the coupled team.
  minor comment
  Bugfix for the IEFLX fixer

Conflicts:
	components/cam/src/physics/cam/phys_control.F90
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tangq tangq Awaiting requested review from tangq

Assignees

@singhbalwinder singhbalwinder

Labels
Atmosphere BFB PR leaves answers BFB bug fix PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

minor bug in atm IEFLX code
5 participants
@kaizhangpnl @rljacob @singhbalwinder @tangq @susburrows