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

Fixes conservation problem for non-water species #2883

Merged
merged 1 commit into from
Jun 14, 2019

Conversation

beharrop
Copy link
Contributor

All non-water tracers are kept as dry mixing ratios, but CLUBB and the
gravity wave parameterizations assume wet mixing ratios, and break
conservation when they are called. This fix converts dry mixing ratios
to wet mixing ratios prior to calling the routines that need wet mixing
ratios. The code fixes follow similar fixes from CESM2, with additional
modifications for E3SM-specific components.

Fixes #2704

[Non-BFB] - Non Bit-For-Bit
[CC] - Climate Changing

All non-water tracers are kept as dry mixing ratios, but CLUBB and the
gravity wave parameterizations assume wet mixing ratios, and break
conservation when they are called. This fix converts dry mixing ratios
to wet mixing ratios prior to calling the routines that need wet mixing
ratios. The code fixes follow similar fixes from CESM2, with additional
modifications for E3SM-specific components.

Fixes #2704

[Non-BFB] - Non Bit-For-Bit
[CC] - Climate Changing
@beharrop
Copy link
Contributor Author

As noted in the commit message, this bug fix produces changes to the climate. Two 5 year F case simulations were run (with and without the fix) and the RESTOM difference is -0.41 W/m2. The full e3sm_diags comparison of the two runs is here: https://portal.nersc.gov/project/acme/beharrop/e3sm_bugfix_test_1year/viewer/

@PeterCaldwell
Copy link
Contributor

I'm surprised the RESTOM difference is so big. My recollection is that this change had less impact on CESM. Do you recall if this is correct? Did you expect to see such large changes? Also, a quick gander of the diags shows that the change is from +0.2 W/m2 in the SWCF and +0.2 W/m2 change in LWclr. Also, I couldn't really see any pattern in terms of which regions had larger or smaller TOA fluxes. Can you articulate any sort of logic for the changes between simulations? Perhaps we need to run for longer than 5 yrs (which is in itself weird).

@PeterCaldwell
Copy link
Contributor

Oh, and I forgot to say - great work! Thanks for taking this important task on!

@beharrop
Copy link
Contributor Author

@PeterCaldwell, I am not sure how large the RESTOM difference was for CESM2. I agree that there isn't much of a discernible pattern in the fields (maybe something from dust). I suppose there could be a smaller, more global change that doesn't show up with this colorbar, but I think that's clutching at straws. I don't have a good suggestion for what is driving these changes. I can run out several more years to see if the signals persist.

@swrneale
Copy link

The difference CESM2 had was not of this magnitude as we decided to move forward with CMIP experiments after fixing the bug.

@beharrop
Copy link
Contributor Author

beharrop commented May 3, 2019

@PeterCaldwell, the 15 year runs produced a RESTOM difference of -0.009 W/m2 (https://portal.nersc.gov/project/acme/beharrop/e3sm_bugfix_test_15year/viewer/). I went back and looked a bit more at how the RESTOM could be so different for a 5 year run and realized I previously linked a 1 year climo instead. The 5 year RESTOM difference is -0.117 W/m2. I suppose we can strike the "climate changing" tag from the commit message. This also puts our response to the bug fix in line with what CESM2 saw.

@PeterCaldwell
Copy link
Contributor

Great! Thanks for extending the run...

@rljacob
Copy link
Member

rljacob commented May 7, 2019

So is this ready to merge?

@rljacob rljacob added the CC PR is climate changing label May 9, 2019
@beharrop
Copy link
Contributor Author

This is ready to merge from my view. We are also going to need these changes to move to maint-1.1. Do I need to issue an additional pull request for that?

Also, we are working on a related issue for the co2 tracers pointed out to us by Keith Lindsay at NCAR (this bug fix does not preserve the near-constant profile of co2). Do you want me to try to combine those changes into this pull request or leave them for a separate PR later?

@rljacob
Copy link
Member

rljacob commented May 14, 2019

Separate PRs for each of those.

@beharrop
Copy link
Contributor Author

Sounds good. The only other thing that needs discussion is whether this is really climate-changing or not. The RESTOM difference for 15 years was -0.009 W/m2. Is there a cutoff for what we consider to be a change in climate for this label?

@rljacob
Copy link
Member

rljacob commented May 30, 2019

@golaz do you have an opinion on the above question?

@beharrop
Copy link
Contributor Author

beharrop commented Jun 6, 2019

@polunma or @PeterCaldwell, do either of you object to calling this change non-BFB instead of CC?

@wlin7, do you have any objections to getting this merged?

@wlin7
Copy link
Contributor

wlin7 commented Jun 11, 2019

@beharrop , I think it is ready to merge. Hard to say this PR is a CC. @rljacob , please remind me, procedure-wise, any difference in handling of a regular non-BFB vs. a CC?

@rljacob
Copy link
Member

rljacob commented Jun 13, 2019

@wlin7 there's no difference. It should go in by itself since so many tests will have answers change.

@wlin7
Copy link
Contributor

wlin7 commented Jun 13, 2019

Thanks @rljacob . When is a good time to merge this non-BFB PR?

@rljacob
Copy link
Member

rljacob commented Jun 13, 2019

Nothing's been merged to next today so now would be a good time. Let the other integrators know by sending a message to the mailing list.

@wlin7
Copy link
Contributor

wlin7 commented Jun 13, 2019

Hi @rljacob , need help on this one.
Here it says on conflict for the PR. But on merging to next, it reports

CONFLICT (file/directory): There is a directory with name components/cam/src/physics/rrtmgp/external in origin/beharrop/atm/tracer_cons_bugfix. Adding components/cam/src/physics/rrtmgp/external as components/cam/src/physics/rrtmgp/external~HEAD

How to handle this? Thanks.

@oksanaguba
Copy link
Contributor

oksanaguba commented Jun 13, 2019

If I could comment on this ~HEAD message: it happened to me for one of PRs; after merge I compared folder in next with merged PR with master folder and there were no diffs. then I did git commit to mark resolution of the conflict. I also ran git stat or something like that to make sure I am not committing anything from that folder.

@wlin7
Copy link
Contributor

wlin7 commented Jun 13, 2019

Thanks for the comments, @oksanaguba. I did similar comparison after the merge. No diffs between the rrtmgp/external in the next after the merge with that in the master. There is no .../rrtmgp/external~HEAD exists as the CONFLICT message indicated. The content under that directory is from an external module and is not part of this PR. I feel it is safe not to do anything about it.

You said "did git commit to mark resolution of the conflict". Do you think in this case I can also just manually issue "git commit" to complete the merge process, essentially ignoring this CONFLICT complaint?

@oksanaguba
Copy link
Contributor

I think without git commit your merge is not complete? you can always toss commit and start over. it is my best recollection that I did git commit and, yes, in my case there was no ~HEAD folder. I assume it just means the folder in question was taken from your HEAD, that is, from next. I found my message for slack i merged 2 PRs to master; like yesterday, merge of one of them, from a fork, created a conflict message for one of submodules. it was something about renaming except that there was no renaming in merge. i double checked with git show --stat that i was not going to commit anything into submodules.

@rljacob
Copy link
Member

rljacob commented Jun 13, 2019

When you updated next, did you also do "git submodule update --init" before you did your merge?

@wlin7
Copy link
Contributor

wlin7 commented Jun 13, 2019

@rljacob , I just switched to next from another branch to do the merge. Didn't do "git submodule update --init" this time. But I don't remember if I had to do it previously when testing certain PR on next.

@wlin7
Copy link
Contributor

wlin7 commented Jun 13, 2019

It wouldn't let me run "git commit" at this point.

#git commit

U       components/cam/src/physics/rrtmgp/external
error: Committing is not possible because you have unmerged files.
hint: Fix them up in the work tree, and then use 'git add/rm <file>'
hint: as appropriate to mark resolution and make a commit.

Can I just remove the whole components/cam/src/physics/rrtmgp/external and do a git rm?

wlin7 added a commit that referenced this pull request Jun 14, 2019
All non-water tracers are kept as dry mixing ratios, but CLUBB and the
gravity wave parameterizations assume wet mixing ratios, and break
conservation when they are called. This fix converts dry mixing ratios
to wet mixing ratios prior to calling the routines that need wet mixing
ratios. The code fixes follow similar fixes from CESM2, with additional
modifications for E3SM-specific components.

Fixes #2704

[Non-BFB] - Non Bit-For-Bit
[CC] - Climate Changing
@wlin7
Copy link
Contributor

wlin7 commented Jun 14, 2019

Merged to next.

I did the following to resolve the issue mentioned above ( ..external~HEAD directory

1. Abort the merge using "git merge --abort"
2. rm -r components/cam/src/physics/rrtmgp/external (not empty,  from previous git subbmodule update --init)
3. merge the PR as usual

@wlin7
Copy link
Contributor

wlin7 commented Jun 14, 2019

Requests submitted to bless 36 DIFFs on sandiatoss3 and 7 on melvin.

@wlin7 wlin7 merged commit ee0852c into master Jun 14, 2019
wlin7 added a commit that referenced this pull request Jun 14, 2019
All non-water tracers are kept as dry mixing ratios, but CLUBB and the
gravity wave parameterizations assume wet mixing ratios, and break
conservation when they are called. This fix converts dry mixing ratios
to wet mixing ratios prior to calling the routines that need wet mixing
ratios. The code fixes follow similar fixes from CESM2, with additional
modifications for E3SM-specific components.

Fixes #2704

[Non-BFB] - Non Bit-For-Bit
[CC] - Climate Changing
@wlin7
Copy link
Contributor

wlin7 commented Jun 14, 2019

Merged to master.

singhbalwinder added a commit that referenced this pull request Jul 26, 2019
Modifications to preserve the vertical shape of CO2 tracers

Follows suggestions by Keith Lindsay for modifying the fix
to conserve non-water tracer species (#2883 ) so that the
nearly constant profile of CO2 is preserved.

Non-BFB when CO2 tracers are active.

[BFB]

* beharrop/atm/co2_profile_fix:
  Modifications to preserve the vertical shape of CO2 tracers
singhbalwinder added a commit that referenced this pull request Jul 29, 2019
Modifications to preserve the vertical shape of CO2 tracers

Follows suggestions by Keith Lindsay for modifying the fix
to conserve non-water tracer species (#2883 ) so that the
nearly constant profile of CO2 is preserved.

[NBFB]

* beharrop/atm/co2_profile_fix:
  Modifications to preserve the vertical shape of CO2 tracers
@kaizhangpnl kaizhangpnl added the non-BFB PR makes roundoff changes to answers. label Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Atmosphere bug fix PR CC PR is climate changing non-BFB PR makes roundoff changes to answers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conservation problems for non-water species
8 participants