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

Coupled BGC enabling and enhancements to ocean BGC #1801

Merged
merged 7 commits into from
Oct 4, 2017

Conversation

maltrud
Copy link
Contributor

@maltrud maltrud commented Sep 21, 2017

This PR defines several coupled BGC (CBGC) compsets. in order to do this, enhancements in the ocean/sea ice BGC codes and configuration scripts were needed. in addition, there are some ocean BGC bug fixes and diagnostic enhancements.

Copy link
Contributor

@jonbob jonbob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this with PET_Ln9.T62_oQU240.GMPAS-NYF.edison_intel and it all looks good. Only things that snuck in and maybe shouldn't are:

  • env_mach_specify.py? I don't think we want to mess with that

  • in mpas-o/cime_config/buildnml, you change the restoring file for oEC60to30v3?

  • in mpas-o/cime_config/buildnml, you take out he "activeTracers" var_array?

@maltrud
Copy link
Contributor Author

maltrud commented Sep 21, 2017

@jonbob thanks for pointing out those differences.

  • env_mach_specific.py: the "module list" command failed for some reason on grizzly so i got rid of it. definitely should be restored to original and i'll just deal with it when on grizzly.

  • mpas-o/cime_config/buildnml: i wanted to make sure i wasn't using the old file with December SSS set to zero. this should also be restored to the original since it is the corrected file.

  • mpas-o/cime_config/buildnml: we want all tracers to be output by default. if no BGC tracers are enabled then this includes just the activeTracers, so no functionality is lost.

@@ -112,7 +112,8 @@ def list_modules(self):
source_cmd = ""

if (module_system in ["module", "module_lmod"]):
return run_cmd_no_fail("%smodule list" % source_cmd, combine_output=True)
# return run_cmd_no_fail("%smodule list" % source_cmd, combine_output=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here?

Copy link
Member

@jgfouca jgfouca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restore list_modules plz.

Copy link
Contributor

@jonbob jonbob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issues found during initial review are fixed

@jonbob
Copy link
Contributor

jonbob commented Sep 21, 2017

@jgfouca - I saw that as well. It should be fixed now, if you want to update your review

@jgfouca
Copy link
Member

jgfouca commented Sep 21, 2017

@maltrud could you describe the error you were getting from list_modules?

@susburrows
Copy link
Contributor

susburrows commented Sep 21, 2017

@maltrud and @jonbob , thanks for getting this in! Just want to make sure @rljacob et al are aware that this PR is critical for the CBGC team and it would be good to get it merged as soon as possible -- ideally before the next batch of CIME updates. We would like to have further testing, development, and performance optimization of the CBGC configurations running from master instead of a feature branch, which should make things easier and more efficient moving forward.

@maltrud
Copy link
Contributor Author

maltrud commented Sep 21, 2017

@jgfouca here's what happens if i don't remove the "module list" bit:
mm@gr-fe1.lanl.gov {1006}% ./case.setup
Machine/Decomp/Pes configuration has already been done ...skipping
Calling drv buildnml

….etc….

Finished creating component namelists
ERROR: Command: 'module list' failed with error '/bin/sh: module: command not found'

since it isn't at all necessary, i just remove it and am able to proceed with no problems.

@jgfouca
Copy link
Member

jgfouca commented Sep 21, 2017

@maltrud it sounds like the machine is configured incorrectly. It should be using modulecmd, not the module bash function.

@susburrows
Copy link
Contributor

@rljacob it wouldn't hurt to have a tag so that we have a clear idea of which version we're referring to. Do you have a specific naming convention for the tags?

@rljacob
Copy link
Member

rljacob commented Sep 21, 2017

@susburrows
Copy link
Contributor

Thanks! Since we are on beta tags now, does that mean we would add a beta tag for this version? Since there are new bug fixes and enhancements in this version, maybe an alpha tag would be more appropriate?

@rljacob
Copy link
Member

rljacob commented Sep 21, 2017

It would be a beta tag. We've left alpha for v1.0.

@susburrows
Copy link
Contributor

OK, so then it would just be the next increment of the beta tag?

Do you have a strategy for sequencing the currrent PRs? I know @maltrud is really eager to get this in before a lot of additional changes (particularly in CIME) go into the model that might make merging more difficult.

@rljacob
Copy link
Member

rljacob commented Sep 22, 2017

Land model has a bunch of PRs they need first but some are non-BFB so will take a while. Is this really BFB? Could go in next week.
Don't fear the CIME changes. run_acme and the test suites are all tested with it and doing well.

@rljacob
Copy link
Member

rljacob commented Sep 22, 2017

You should probably look at the pending land model PRs and see if you need them. Many are bug fixes which I assume would impact BGC in some way.

@susburrows
Copy link
Contributor

@rljacob , we talked about this in the CBGC meeting and the consensus seems to be that we should wait until all PRs (this one and the pending land PRs) are in, and then create a tag. Also, since this PR modifies ocean and sea ice, can it be merged in parallel to merging the land PRs?

@rljacob
Copy link
Member

rljacob commented Sep 25, 2017

Sure. This PR has the "Bug fix" label. What bug does it fix? (Should be an issue in github.)

@jonbob
Copy link
Contributor

jonbob commented Sep 25, 2017

No bug -- that was probably just included by mistake from the issue list

@maltrud
Copy link
Contributor Author

maltrud commented Sep 25, 2017

there were a couple of small things (like a unit conversion issue) that i noticed as i was putting together the branch, so i added Bug Fix. but no issues to link to. sorry about that.

@rljacob rljacob added this to the v1.0beta3 milestone Sep 26, 2017
jonbob added a commit that referenced this pull request Oct 2, 2017
Coupled BGC enabling and enhancements to ocean BGC

This PR defines several coupled BGC (CBGC) compsets. in order to do
this, enhancements in the ocean/sea ice BGC codes and configuration
scripts were needed. in addition, there are some ocean BGC bug fixes and
diagnostic enhancements.

Tested with:
* ERS_Ld5.T62_oEC60to30v3.GMPAS-OECO-IAF.cori-knl_gnu

[NML]
[BFB]
@jonbob
Copy link
Contributor

jonbob commented Oct 2, 2017

Merged to next

jgfouca pushed a commit that referenced this pull request Oct 3, 2017
Test suite: scripts_regression_tests.py, PRE.f09_f09_ADESP_TEST.hobart_nag,
            PRE.f19_f19.ADESP_TEST.hobart_nag
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes #1801

User interface changes?:

Update gh-pages html (Y/N)?:

Code review:
jgfouca pushed a commit that referenced this pull request Oct 3, 2017
Remove mask specification for test using DOCN%SOMAQP.
The mg16 and mg17 grid has a hole over Greenland. This was causing Nans when
running SOM aquaplanet at f09_f09_mg17. The fix is not to specify a mask when using
DOCN%SOMAQP.

Test suite: scripts_regression_tests.py, PRE.f09_f09_ADESP_TEST.hobart_nag,
PRE.f19_f19.ADESP_TEST.hobart_nag
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes #1801

User interface changes?:

Update gh-pages html (Y/N)?:

Code review: jedwards
@jonbob jonbob merged commit 09f664c into master Oct 4, 2017
jonbob added a commit that referenced this pull request Oct 4, 2017
Coupled BGC enabling and enhancements to ocean BGC

This PR defines several coupled BGC (CBGC) compsets. in order to do
this, enhancements in the ocean/sea ice BGC codes and configuration
scripts were needed. in addition, there are some ocean BGC bug fixes and
diagnostic enhancements.

Tested with:
* ERS_Ld5.T62_oEC60to30v3.GMPAS-OECO-IAF.cori-knl_gnu

[NML]
[BFB]
@jonbob
Copy link
Contributor

jonbob commented Oct 4, 2017

merged to master

@jonbob jonbob deleted the maltrud/ocean/CBGC_initial_version branch October 4, 2017 17:37
@susburrows
Copy link
Contributor

Great news, thank you Jon!

@jonbob
Copy link
Contributor

jonbob commented Oct 4, 2017

Well @susburrows and @maltrud, you all have worked a long time to get this done -- glad I could be part of it

@maltrud
Copy link
Contributor Author

maltrud commented Oct 4, 2017

yes, indeed. thanks @jonbob for all of your help with this. now for the next round of updates.....

@mark-petersen
Copy link
Contributor

Yes, thanks to everyone. After a long summer, the log jam is finally clearing.

jgfouca pushed a commit that referenced this pull request Oct 25, 2017
Coupled BGC enabling and enhancements to ocean BGC

This PR defines several coupled BGC (CBGC) compsets. in order to do
this, enhancements in the ocean/sea ice BGC codes and configuration
scripts were needed. in addition, there are some ocean BGC bug fixes and
diagnostic enhancements.

Tested with:
* ERS_Ld5.T62_oEC60to30v3.GMPAS-OECO-IAF.cori-knl_gnu

[NML]
[BFB]
jgfouca pushed a commit that referenced this pull request Feb 27, 2018
Coupled BGC enabling and enhancements to ocean BGC

This PR defines several coupled BGC (CBGC) compsets. in order to do
this, enhancements in the ocean/sea ice BGC codes and configuration
scripts were needed. in addition, there are some ocean BGC bug fixes and
diagnostic enhancements.

Tested with:
* ERS_Ld5.T62_oEC60to30v3.GMPAS-OECO-IAF.cori-knl_gnu

[NML]
[BFB]
jgfouca pushed a commit that referenced this pull request Mar 14, 2018
Coupled BGC enabling and enhancements to ocean BGC

This PR defines several coupled BGC (CBGC) compsets. in order to do
this, enhancements in the ocean/sea ice BGC codes and configuration
scripts were needed. in addition, there are some ocean BGC bug fixes and
diagnostic enhancements.

Tested with:
* ERS_Ld5.T62_oEC60to30v3.GMPAS-OECO-IAF.cori-knl_gnu

[NML]
[BFB]
rljacob pushed a commit that referenced this pull request Apr 12, 2021
Test suite: scripts_regression_tests.py, PRE.f09_f09_ADESP_TEST.hobart_nag,
            PRE.f19_f19.ADESP_TEST.hobart_nag
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes #1801

User interface changes?:

Update gh-pages html (Y/N)?:

Code review:
rljacob pushed a commit that referenced this pull request Apr 12, 2021
Remove mask specification for test using DOCN%SOMAQP.
The mg16 and mg17 grid has a hole over Greenland. This was causing Nans when
running SOM aquaplanet at f09_f09_mg17. The fix is not to specify a mask when using
DOCN%SOMAQP.

Test suite: scripts_regression_tests.py, PRE.f09_f09_ADESP_TEST.hobart_nag,
PRE.f19_f19.ADESP_TEST.hobart_nag
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes #1801

User interface changes?:

Update gh-pages html (Y/N)?:

Code review: jedwards
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants