-
Notifications
You must be signed in to change notification settings - Fork 25
2018 meeting notes
Suggestions for cleaning up the marbl-diags repository:
- Rename
AnalysisElement? The term "element" is misleading, but no better suggestion came up - Defining a
Dataset:- do not need to be on same grid
- do need to have same variables
- similar time dimension
-
referenceas a property of theAnalysisElementinstead of in theDataSet(why haveroleat all?) - Look into using cf names for
variable_list -
variable_dictcould be hard-coded in class definitions - Is there a way to provide depth-dependent contour levels?
Also talked a little bit about #327: plan is to accept the PR after CIME is updated to use PGI 18.10 or later on hobart. Assumption is that the DOE machines running CESM are using the latest PGI compiler so this won't be in an issue, but that should be verified.
Per this comment, all that remains in #53 is to rewrite the particulate computation routines... there is some k-level dependency between compute_scavenging(), compute_large_detritus_prod(), and compute_particulate_terms() so the plan is to combine those three routines will into one. This marks the last merged pull request before CESM 2.1.0 is released, though the documentation still needs to be updated.
For the rest of 2018, we'll set aside one meeting a month to talk science rather than software. Beginning in January, 2019, the software meeting will be biweekly, and weeks without a software meeting will have a science meeting. Notes from the science meetings will not be posted on this wiki.
Code-wise, getting close to being done with k-loop migration.
Mike brought up the possibility of using method objects rather than subroutines in marbl_interior_tendencies_mod to compute intermediate values used for the local tendency computation. Decided not to do so -- from a code-readability standpoint, it's nice having everything contained in marbl_interior_tendencies_mod rather than spread across multiple modules. So that will not be incorporated in #318.
Most subroutines are coming out of the do loop easily, but compute_scavenging(), compute_large_detritus_prod(), compute_particulate_terms(), and update_particulate_terms_from_prior_level() need to be combined into a single subroutine before pulling out of the k-loop.
More discussion re: Externals_POP.cfg and the PR template. Also, Mike will get in touch with CGD's ISG group to talk about backing up the marbl-ecosys/MARBL repository (wiki, issue tickets / PRs, release notes, etc)
We should make fewer development tags, but send more emails when we do make tags. One possibility would be to have Externals_POP.cfg on the marbl_dev branch of POP point to specific git commit ids instead of tags -- tags would only be made before updating the version of MARBL used by the POP trunk. Mike will look into whether manage_externals can do that (and if not, he'll submit an issue ticket).
Also, we'll look into adding a template for pull requests that will also be used as the merge commit message. This will improve the messages in git log --first-parent, making it more like a change log.
Development priority is #53 -- pushing the k loop from marbl_interior_tendencies_compute() into each of the compute_*() subroutines.
Keith's focusing on some CAM issues that need to be resolved for CMIP development, so work on #272 is not a priority right now. Mike will spend 8 - 12 hours getting up to speed on the IOMB process.
Mike will spend a little bit of time getting spun up with the IOMB workflow on cheyenne, but it's not as high a priority as MARBL clean-up. There are also some science changes for MARBL in the pipeline:
- Keith's work on shadow tracers for Newton-Krylov spin-up
- Some infrastructure changes that could make it easier for Jessica to bring in size-structured PFTs
- Making it easier to enable coccolithophores as a fourth autotroph
We need to work out how / when those will be brought in (probably after the CESM 2.1.Z release used for the majority of CMIP6 runs is out)
Discussed proper place for the comments that outlined the different ecosys_mod.F90 versions -- rather than keep these comments in the Fortran code, new pages should be added to the sphinx documentation. (Issue #310.)
This also raised the issue of "what comments do we want in the Fortran module header?" MOM6 uses doxygen to generate API documentation, and they put the comments regarding the general purpose of the module in the footer of the F90 file. We'd like a better way to tie the API documentation to the sphinx documentation page, though - maybe there is a doxygen extension for sphinx?
Mike will prioritize the bit-for-bit changes we want to make so that the differences between the version of MARBL in CESM 2.1 and the eventual MARBL 1.0.0 release are minimal. We expect a lot of community-provided pull requests to be based off CESM 2.1, and hope to minimize conflicts in accepting those changes.
After the meeting, Mike talked to Gokhan -- answer-changing bug fixes (such as fixing issues with the Robert Filter in POP or the cksi bug in MARBL) will NOT be back-ported to CESM 2.0. So CESM 2.1 won't be able to reproduce CESM 2.0 results in a bit-for-bit fashion, even though MARBL provides the old tunings.
Once the CMIP6 tunings are finished, those will get merged onto MARBL's development and POP's trunk. #297 will go on top of that.
Meanwhile, Mike will work on a code consistency checker -- focus will be on the easy tests (line length, white space, etc) rather than getting bogged down by harder tests (indentation). After CMIP6 is setup and CESM 2.0 defaults are available, we'll focus on cleaning up as much code as possible ahead of the CESM 2.1 release. This puts the single time-step test on the back-burner until after the release.
We will host the documentation on github pages, renaming sphinx_requirements.txt to py_requirements.txt and making sure we pull in correct version of the theme. Also, Travis CI can run pip install -r docs/py_requirements.txt so we can test the documentation built as part of our continuous integration. This means accepting #284 and closing #285.
Keith and Mike talked about coding requirements, and Mike will update the strawman rules based on some comments. Also looked through the MARBL documentation for CESM 2.0 and cleaned up quite a bit of it.
Gokhan and Mariana shared timeline for CESM 2.1 -- we were worried about MARBL development stalling, but CESM 2.1 release window is pretty short (mid- or late-September). Also talked about MARBL documentation for CESM 2.0, and Mike will work with Alice B to get that up and running.
To-do:
- clean up developer's guide prior to CESM 2.0 release
- break up generic "code clean-up" issues into discrete individual tickets
- Look into TravisCI to enforce code consistency
Mike will send everyone with read access to NCAR/MARBL an email informing them of the move to public development and the marbl-ecosys/MARBL repository. Email will go out May 23, and the move to the marbl-ecosys organization will hopefully happen on May 29.
Maintaining a ChangeLog for the development tags is not necessary - developers should be able to rely on git diff to see what changed between two tags and git log to see the first tag that contains a specific commit. Moving forward, the metadata Mike was putting in the "release notes" for development tags will go in the commit message when merging a branch onto development. Release notes will only be used for tags on stable or on release branches.
The frequency of tags on development has not yet been determined. Perhaps our current "tag every pull request" method is too frequent? We will develop a rule-of-thumb to determine which pull requests should be tagged.
Lots of discussion on branching and tagging. We want a long-lived stable branch (default when cloning repository), and GCM-specific releases will also have branches that start from a commit on stable.
stable | cesm2.0 | cesm 2.1
----------------------------------------
v1.0.beta01 | cesm2.0_n00 |
v1.0.beta02 | cesm2.0_n01 |
v1.0.beta03 | | cesm2.1_n00
| cesm2.0_n02 |
v1.0.0 | |Things we need before going public:
- License
- Accurate documentation
Soon after going public:
- TravisCI for testing (python at least) as part of pull request
- Read the Docs for documentation (want to have multiple versions of documentation available)
We definitely want to use the marbl-ecosys organization rather than just making NCAR/MARBL public (so make NCAR/MARBL public and then transfer)
Discussion around the idea of going public for development -- would we still have separate development and release repositories? Consensus was no, have a single repository that come up with a branch / tag naming convention that (a) gives the user the latest release code by default, and (b) makes it clear that a user is switching to development code.
Also, an open question: do we want the latest release to be able to run with the same scientific configuration as old releases? E.g. should v2.0.0 be able to generate v1.0.0-esque solutions?
Discussed potential release paths for two different options:
- private development repo + public release repo [we have been talking about keeping development private because we are not comfortable with users relying on un-vetted science]
- public development and release (two ways to implement)
- one repository (
masterbranch for releases, and adevbranch for development) - two different repositories (separate development repository that is public, but releases would be mirrored in a separate repository without any of the development branches or tags)
- one repository (
We will need to figure this out soon (between May 11 code freeze and June 1 CESM release)
Fixing the POP%prod bug in marbl0.28.8 highlighted another issue: zooplankton (with a fixed P:C ratio) grazes on autotrophs that can have a much lower P:C ratio. When this happens, the zooplankton pulls more P out of the column to maintain its ratio, and this can occasionally drive POP%prod to negative values. There was a bug where POP%prod was reset to zero when POC%prod was negative, rather than forcing POP%prod to be non-negative, and fixing this bug started causing occasional mass-balance failures in Jint_Ptot. We discussed possible fixes to the newly uncovered bug, such as drawing P from somewhere else or dumping C back into the column. This is holding up the POP tagging process, and will hopefully be resolved soon.
MARBL releases will be numbered X.Y.Z, where X is the science version number, Y is the API version number, and Z is a patch version number. There is no indication about whether a patch release is bit-for-bit with its predecessor or not.
MARBL development will add a forth digit with the beta prefix, and the location of this digit indicates what tag is being developed:
-
X.Y.Z.betaN1is developing the patch releaseX.Y.Z -
X.Y.betaN1.N2is developing the API releaseX.Y.0; all tags whereN1is the same share the same API -
X.betaN1.N2.N3is developing the science releaseX.0.0; all tags whereN1andN2are the same share the same API
We will have a very broad definition of API.
Release strategy: the development repository will maintain release branches for each CESM version (and other models that need to make the code public). Following a successful control run, this branch will also be pushed to the public repository.
We have decided not to use semantic versioning, as it does not seem appropriate for scientific software. Mike will work on a versioning scheme to use instead, and we will continue with the current v0.X.Y approach in development.
Most of the time was spent discussing the development / release plan for the code. We are hoping to continue to develop in a private repository, and only make [publicly available] release tags for updates that have been scientifically vetted - i.e. have been used in a control run. Mike will put together a more detailed proposal over the coming weeks.
We also talked about merging vs rebasing - current decision is to continue to merge branches back to the trunk, but use the --no-ff option so that git log --first-parent accurately tracks master. We do NOT recommend rebasing, as it effectively edits history. Rebasing would make it impossible to rerun a test using an old code base (unless we keep branches indefinitely) and also makes it harder to track when changes were actually made.
CESM 2 plan: in the next week, Mike will focus on the quick fix issues (#240, #241, #244, and #248) and Keith L will work on an fe_bioavail fix from Keith M. After the freeze, Mike will fix the ecosys_restart_file bug (#246) and Keith L will be responsible for #105, #218, and #211.
We also talked about how to make MARBL available for CESM 2.0; we definitely want POP to pull MARBL down from a git repository, but that opens up a can of worms:
- MARBL development is private, and our test suite doesn't have enough coverage for us to be comfortable making the entire development process public (Keith L and Mike would still like private forks from which to submit pull requests)
- The current implementation of the
manage_externalswill produce an unhelpful error when a subversion repository has been replaced with a git repository - We haven't clearly defined a workflow for developing MARBL once it has been made public, but our current workflow is not set up to support old versions of code; so we need to make sure that it is easy to provide updates to the version of MARBL that is used in CESM 2.0.0 (and hopefully also accept pull requests that are developed using the version of MARBL in CESM 2.0)
Talks at CESM Workshop:
- OMWG talk geared towards changes since CESM 1.2... how to enable CISO code, how to set MARBL parameters, and how to change BGC-related diagnostic output
- SEWG talk geared towards coding details... how POP's build scripts interact with MARBL's python scripts, how POP Fortran calls MARBL code. Emphasize MARBL independence (no reliance on
build-namelistor other POP tools)
Maybe instead of another tutorial we could convince interested parties to help us when we reach the code clean-up / documentation step prior to MARBL 1.0.0 release?
Collaborating with CFP might be useful, but the real question is "is it worth the time?" If all we are doing is pulling marbl_logging_mod.F90 and marbl_utils_mod.F90 into their own software package and we don't envision much development happening there, then why spend the time? We can just let CFP copy those modules and hand-edit them.
Lastly, there are no longer open issues with no milestone.
Most of the discussion was looking several steps ahead in my work to create a test for set_surface_forcing() and set_interior_forcing(). Some key points:
- We should do a better job organizing the
tests/directory:-
init-twiceis a unit test, in the sense that it really testsmarbl_instance%shutdown()to ensure we are deallocating everything that has been allocated. - Once
set_forcing(which should be renamedcompute_tendenciesor something) is finished, we don't need separate a regression test forinit(we can print the MARBL log in the full test and then changes in init will result in differences in standard out) - All the
requested_*tests would be better labelled asexamplesinstead oftests; would this require a completely different executable?
-
- We need to improve the build system to handle the need for netCDF
- require netCDF for test directory (but maybe not for examples)?
- require users to build via python scripts (add
FROM_PYTHON=FALSEto Makefile, overwrite from python scripts, abort unlessFROM_PYTHON == TRUE) -
nc-configmay not be consistent enough across machines to use it to get include / linking flags (nc-config --flibsdoes not give reasonable results when using homebrew to install on a Mac; other options did not play nicely on hobart)
- If we rely on python for the build, could also use config files (read by python, passed to
make) to store machine-specific settings (and also state of current build)
Ahead of the MARBL v1 release, I should sort remaining MARBL 1.0.0 issues into "API changing" and "not API changing"; it may make sense to push some of the non-API-changing issues back until after the release and focus on finalizing the interface. Speaking of API changes, we should probably include both short name and long name in the forcing metadata type (instead of varname, which is currently what we provide to the GCM).
For the jupyter notebook generating forcing data sets, Matt recommended using xarray instead of netCDF4... also, besides the southern ocean data point a location in the eastern equatorial Pacific would be interesting. lat_in and lon_in should be lists, but each individual column should be written to its own netcdf file.
Jessica's issues will be addressed, and some in-ticket comments were made but no clear priority decided. Currently the python code processing templated diagnostics in YAML assumes one template replacement per variable, so something like {auto}_graze_{zoo}_to_{zoo} would require python overhaul. We also discussed idea of having multiple POC pools, which would be a huge undertaking because of the effect it would have on routing.
For the set_forcing test, besides outputting tracer tendencies and surface forcing values, I'll also include diagnostics. For now I'll just output all computed diagnostics, though it might be nice in the future to have an optional argument to the test akin to --input-file to allow users to customize the diagnostic output.
Now that the low-hanging fruit has been dealt with, we talked about how to move on to the next big issue -- improving the functionality of marbl_domain_type. We decided that it made the most sense to do the following:
- Create a stand-alone test for
set_surface_forcingandset_interior_forcing - Use this new test to make sure we only loop from 1 to
kmt(lots of loops currently run tokm, which isn't necessary) - Once all loops are the right size, we can work on the interface to allow GCMs where
k = 1is the bottom of the column rather than the surface.
More talk on #1 (aborting if mass balance fails). This would have saved Jessica a lot of time when she stumbled upon bug #224; current plan would be to introduce an assert_near_zero() function. Keith would like to hard-code in the tolerance, which will require looking at current POP output to determine how big each of the mass balance terms can be. This is preferable to determining tolerance on the fly, because the latter might mask some changes we would like to see. (Also, we can't necessarily use the 100m integral as a basis for determining the tolerance because if the column is <100 m deep then the mass balance = the 100 m integral).
Testing the diagnostics with Jessica's size-structured set-up highlighted a bug with how logical settings are handled - e.g. length of tracer_restore_vars depends on whether ciso_on = ".true." but some users would set ciso_on = "T" instead. (Solution is to convert all acceptable Fortran logical values to either ".true." or ".false." so the comparison is always valid.) We also discussed combining POP and MARBL diagnostics into a single file (ecosys_diagnostics).
My plan once the diagnostics work is on the trunk will be to go back and address several tickets that seemingly have quick fixes; this should coincide nicely with everyone else being at Ocean Sciences.
The majority of time in this meeting was spent digging into the weeds of the YAML / python setup for defining diagnostics. Lots of good suggestions, such as better implementation of templating (things like ((autotroph_sname)) for autotroph short name) and not introducing an additional level of dictionaries just to handle per-autotroph and per-tracer diagnostics.
PGI continues to introduce compiler bugs that prevent MARBL from running (which is actually an improvement from the days where PGI compiler bugs prevented MARBL from building). Need to figure out if dropping PGI support is okay with CESM / E3SM, or if some nominal testing needs to continue as PGI updates its compiler.
Walking through the updates to default_settings.yaml (to include tracer short-names instead of just total count) raised some internal inconsistencies -- marbl_interface_private_types.F90 refers to tracers in all lower-case (po4 and caco3) while marbl_init_mod.F90 actually sets up the tracer metadata using proper atomic case (PO4 and CaCO3). The YAML file will use the marbl_init_mod.F90 case, and hopefully we can auto-generate both marbl_interface_private_types.F90 and marbl_init_mod.F90 based on the YAML so that they will be consistent as well.
We prioritized current open tickets to determine what we want in the CESM2.0 sub-milestone of the MARBL 1.0.0 milestone.
Discussion on how to fix the bug in interior restoring, which should be on master by the next meeting. This fix will definitely go into CESM 2.0, but it does not need to be in the next beta tag... so the fix will get merged onto marbl_dev immediately but not pushed to the trunk just yet.
Not much MARBL-specific conversation, just walked through NCAR/manage_externals and how it will be used in CESM tags following cesm2_0_beta08 (and also in POP starting in whatever trunk tag goes into cesm2_0_beta09)