Remove duplicative allsoils filter for FATES SatellitePhenology call #1515
Conversation
…l with active and inactive filter call
glemieux
added
the
code health
|
Initial testing shows that SMS SatellitePhenology testmod pass b4b against baseline: |
|
Awesome, thanks for this cleanup.
|
|
@rosiealice I adapted your comment from your allsoil filter comment (that we're replacing). Do you think I accurately represent what's being attempted: |
|
Thanks a lot @glemieux . For the future, we prefer if checking off checkboxes is left to the reviewer (or whoever added the checkbox in the first place): see https://github.com/ESCOMP/CTSM/wiki/Pull-request-review-workflow and the links from there for details. (No worries about this: I fully expect to need to tell everyone this one or more times before it sticks, but it's helpful for me as a reviewer to be able to use checkboxes like this as a list of requests I have made, which I can then check off as I'm satisfied that they have been addressed. Please feel free to give me any feedback about how this could be more intuitive. For many contributors this is not an issue, because they wouldn't have permission to check off my checkboxes anyway, but you were able to because you have write permission to this repository.) |
|
@billsacks gotcha. I've been using task check boxes for myself on other PRs and issues so I've just been in the habit of ticking them off when I'm done without much thought. I hadn't reviewed the PR workflow doc in a while (since before you added the gh-pr-query tool) so I missed the direction on how to handle them when being assigned by reviewers. Is there a way to lock down the edit option for comments just to the OP? |
I don't think so. Comments can be edited by the OP or anyone with write access to the repository. That's still better than GitHub's "resolve" feature, which can be triggered by the PR author regardless of their repo permissions, but it means that we need to educate anyone with write permissions on this workflow. (I have been in touch with some people at GitHub to encourage them to add first-class support for a feature like this, but I have no idea if/when that would happen. In the meantime, this admittedly imperfect system is the best I've been able to come up with, but I'm open to suggestions.) |
|
Note to self: given that this effects FATES SP mode only I think we could couple this with the eventual fix for #1485 |
|
Izumi Similarly, Cheyenne The |
First Commit of CN Matrix Solution: Start bringing in the use_matrixcn and use_soil_matrixcn control options for the Carbon/Nitrogen BGC model using the matrix solution methodology developed by Dr. Yiqi Luo and his EcoLab members: Drs. Xingjie Lu, Yuanyuan Huang and Zhengguang Du, at Northern Arizona University Work on Anomaly forcing for SSEP scenarios: Do some work on anomoly forcing for SSP scenarios. Update the script in tools/contrib to work with the latest data and replicate results of Keith Oleson and Sean Swenson. Add some test cases for it. Python Environment Management using Conda: More work on the manage_python_env script to setup the "ctsm_py" conda environment. This now works for UCAR machines (such as cheyenne and casper) as well as CGD machines (like izumi). This script can also be called for tools, and the test tools mechanism is now using it rather than ncar_pylib. ./manage_python_env SCAM Fix: Add checkimport method for CMEPS so that single-column case can work with SCAM, from Jim Edwards. Black Formatter for Python Code: Run "black" python code reformatter on all code under the "python" directory. Add the commits to .git-blame-ignore-revs so they can be ignored when "git blame" is used, if you add this to each clone where you want this to apply: git config blame.ignoreRevsFile .git-blame-ignore-revs You can do it for your worktrees, but since the file only applies to CTSM for ctsm5.1.dev100 and forward, you'll need to do it for each CTSM clone seperately. Also there is now a github action that checks that python code under the "python" subdirectory is black clean. If it's not the action will fail and send you an email about it. Make Map Data Fix: Simple fix from Sam Rabin. Fix for DISPLA on History Output: Fix from Keith Oleson to set history output for DISPLA (displa_patch) to zero when vegetation is buried by snow.
|
After conversation with @ekluzek regarding efforts to address #1794, we've decided to update the externals configuration to include the tag that will result from ESMCI/ccs_config_cesm#50. There will also be a branch tag update for time per ESMCI/ccs_config_cesm#50 (comment) |
Fix accumulation variables when changing model time step Accumulation variables (e.g., 1-day or 10-day averages) were writing and reading their accumulation period (expressed in time steps) to the restart file. This caused incorrect behavior when changing the model time step relative to what was used to create the initial conditions file (typically a 30-minute time step). So, for example, if you are using a 15-minute time step with an initial conditions file that originated from a run with a 30-minute time step (at some point in its history), then an average that was supposed to be 10-day instead becomes 5-day; an average that was supposed to be 1-day becomes 12-hour, etc. (The issue is that the number of time steps in the averaging period was staying fixed rather than the actual amount of time staying fixed.) For our out-of-the-box initial conditions files, this only impacts runs that use something other than a 30-minute time step. Typically this situation arises in configurations with an active atmospheric model that is running at higher resolution than approximately 1 degree. It appears that the biggest impacts are on VOC emissions and in BGC runs; we expect the impact to be small (but still non-zero) in prescribed phenology (SP) runs that don't use VOC emissions. This tag fixes this issue by no longer writing or reading accumulation variables' PERIOD to / from the restart file: this isn't actually needed on the restart file. See some discussion in ESCOMP#1789 for more details, and see ESCOMP#1802 (comment) for some discussion of outstanding weirdness that can result for accumulation variables when changing the model time step. The summary of that comment is: There could be some weirdness at the start of a run, but at least for a startup or hybrid run, that weirdness should work itself out within about the first averaging period. A branch or restart run could have some longer-term potential weirdness, so for now I think we should recommend that people NOT change the time step on a branch or restart run. With (significant?) additional work, we could probably avoid this additional weirdness, but my feeling is that it isn't worth the effort right now. In any case, I feel like my proposed fix will bring things much closer to being correct than they currently are when changing the time step.
|
Cheyenne The fates testmod differences are all due to File location on Cheyenne: Izumi File location on Izumi: |
|
Discussed this with @rgknox at our regular Monday morning PR triage meeting. We decided to take care of the UPDATE: no fates-side fix necessary. Retesting the fates testmods with DIFFs. |
|
Re-running the fates tests that showed differences now reports the appropriate difference value as included in the fates parameter file from the last update: https://github.com/NGEET/fates/blob/def6b3e76f9ff3043150a777f403883b3e659374/parameter_files/fates_params_default.cdl#L800 |
Description of changes
Addresses #1488
This also incorporates a small name change to the
Fatestestmod and thefates-spusermod per NGEET/fates#854.Specific notes
Contributors other than yourself, if any:
CTSM Issues Fixed (include github issue #):
Are answers expected to change (and if so in what way)?
Any User Interface Changes (namelist or namelist defaults changes)?
Testing performed, if any:
(List what testing you did to show your changes worked as expected)
(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on cheyenne for intel/gnu and izumi for intel/gnu/nag/pgi is the standard for tags on master)
NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).