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

Implement ELM v2 data architecture for mass and energy state and flux variables #2742

Merged
merged 44 commits into from
Mar 12, 2019

Conversation

thorntonpe
Copy link
Contributor

Migrates mass and energy state and flux variables from CLM4.5 data structures, which mixed variables at multiple subgrid levels, to ELM v2 data structures, which separate the variables according to subgrid level. For example, carbon state variables are moved from a single type into three distinct types, one for gridcell-level, one for column-level, and one for vegetation (patch)-level variables. Methods acting on these types are also defined independently for each subgrid level. Migrates all energy, water, carbon, nitrogen, and phosphorus state and flux variables and data-type methods.

Leaves open the issue of how to structure subroutine calling interfaces. The original code uses a variety of methods. Here, an example is provided for the module biogeochem/CNCStateUpdate1Mod.F90 and all its upstream calls, based on explicit references to data type instances at the clm_driver() level, and passed references at all lower levels.

Branch tests bit-for-bit against the E3SM land developer test suite.

[BFB]

@thorntonpe
Copy link
Contributor Author

Design document for this work is on the Land/Energy NGD Confluence page, at
https://acme-climate.atlassian.net/wiki/spaces/NGDLE/pages/924451042/Design+Documents+for+new+PRs

@thorntonpe thorntonpe requested review from bpbond and rgknox February 12, 2019 20:27
@rljacob
Copy link
Member

rljacob commented Feb 12, 2019

Given the number of conflicts, this is a case where you should merge master to the branch to help resolve them.

@rljacob
Copy link
Member

rljacob commented Feb 12, 2019

Although you'll have to do all your testing again then.

Adds subroutines to initialize and clean all the new column and
vegetation level data structures. also renames a few physical_properties
types to improve data structure naming consistency

[BFB]
Created a new module for the state and flux variables at the column
level (ColumnDataType). Needed because col_pp is referenced inside
HistFileMod, and initialization of the history fields for state and
flux variables references HistFileMod. Implementation will be extended
later to other sub-grid levels.

[BFB]
An incremental commit in preparation for major update associated with
tsoisno variable.

[BFB]
Includes migration of history initialization and restart for soil and
snow temperatures.

[BFB]
Continued migration of column-level energy state variables, with
history, cold start, and restart.

[BFB]
Continued migration of column-level temperature variables.

[BFB]
Add remaining temperature-related variables to new data type, and
update the use of these variables throughout the code. Unused variables
removed (e.g. dt_grnd_col) or treated as constant (e.g. beta_col).
Also modified the instantiation of urbanparams_type, moving this out
of clm_instMod.F90, and into UrbanParamsType.F90. This fixes some
issues with circular use referencing.

[BFB]
Added initialization, history, and restart capability for vegetation
energy state structure. Also introduced a new module for vegetation data
structures. Tested against land developer test suite

[BFB]
Complete migration of all vegetation (pft) level energy state variables
from the vegetation_vars structure in veg_es structure. Requires a
change to the BeTR submodule for data passing to use new data structures.
With that change, all land developer test suite tests pass BFB.

[BFB]
Add topounit-to-gridcell scaling factor, and ability to accept
history file variables defined at the topounit level.

[BFB]
Created a new data structure for the state and flux variables that are
defined at topounit level. Revised all references to these data structures
in "use" statements.

[BFB]
Completed the addition of new history variables for the v2 data
architecture at the topounit level. All previous history field names
and other metadata are maintained in the new code, so this change is
BFB with master. At a later date, ELM team may want to add additional
history outputs, or remove some that are duplicates with different names
as part of the code cleanup process.

[BFB]
@jqyin
Copy link
Contributor

jqyin commented Mar 7, 2019

I can make the needed commit for the build fail.
@rljacob @bishtgautam what are your thoughts about the DIFF for SMS_Ld2.ne30_oECv3_ICG.A_WCYCL1850S_CMIP6.anvil_intel.allactive-v1cmip6. It's only for one field and really small.

@rljacob
Copy link
Member

rljacob commented Mar 7, 2019

Its your PR so you can declare those diffs to be acceptable and ask for them to be blessed. But you should know why they happened.

@rljacob
Copy link
Member

rljacob commented Mar 7, 2019

The diffs are between coupler history files output at the end of the runs. "Ln9" is 9 timesteps. "Ld2" is 2 days".

@thorntonpe
Copy link
Contributor Author

Regarding the DIFFS: every other time I have seen DIFFS in FSDS they propagate quickly to many other energy variables. The fact that this impacts only FSDS suggests that somehow the DIFF is not propagating. I'd be glad to explore further, but it seems like the simulation is progressing BFB in subsequent time steps.

@thorntonpe
Copy link
Contributor Author

@rljacob just saw your message about time steps. Is there information in the DIFF output that indicates on which timestep the reported DIFF occurs?

@rljacob
Copy link
Member

rljacob commented Mar 7, 2019

I'm going by the name of the files that were compared which is in the teststatus output on cdash. You can also download a tar file of all the log files by clicking on the gold box next to the test name.

Questions to ask yourself: why these cases and not other F or coupled cases?

@thorntonpe
Copy link
Contributor Author

Based on what we know now, I think these DIFFS are acceptable, and would like to have them blessed. I would like to understand why they occur, and can set this as a new task to dig into these compsets and provide an answer. There is something special about how these compsets are working compared to all the other tests, and I'd work with the people most knowledgeable about these tests to sort it out. I would not ask to bless and proceed, even for small DIFFS, if this was propagating to the 30+ other coupler history variables that are typically impacted when a DIFF appears in FSDS.

@rljacob
Copy link
Member

rljacob commented Mar 7, 2019

Ok. @jqyin add your commit for the build fix, re-merge to next and wait for another round of testing.

jqyin added a commit that referenced this pull request Mar 7, 2019
Implement ELM v2 data architecture for mass and energy state and flux variables

Re-merge to fix build error

[BFB]
jqyin added a commit that referenced this pull request Mar 11, 2019
Implement ELM v2 data architecture for mass and energy state and flux variables

Re-merge to fix the floating-point exception for F cases

[BFB]
@jqyin
Copy link
Contributor

jqyin commented Mar 12, 2019

@thorntonpe the build and runtime fails are now clear, but SMS_D_Ld1.ne30_oECv3_ICG.A_WCYCL1850S_CMIP6.sandiatoss3_intel.allactive-v1cmip6 and SMS_D_Ln5.ne4_ne4.FC5AV1C-L.sandiatoss3_intel.cam-cosplite_nhtfrq5 show large DIFFs on 18 fields, e.g.,

CLMMODIS (ncol,time) t_index = 1 1
8094 48602 ( 615, 1) ( 3, 1) ( 41423, 1) ( 41, 1)
46865 1.000000000000000E+02 0.000000000000000E+00 1.0E+02 1.000000000000000E+02 9.2E-02 0.000000000000000E+00
46865 1.000000000000000E+02 0.000000000000000E+00 0.000000000000000E+00 2.000000000000000E+01
48602 ( 607, 1) ( 3, 1)
avg abs field values: 1.283964347839355E+01 rms diff: 7.8E+00 avg rel diff(npos): 9.2E-02
1.321272659301758E+01 avg decimal digits(ndif): 0.4 worst: 0.0
RMS CLMMODIS 7.7778E+00 NORMALIZED 5.9709E-01

CLMODIS (ncol,cosp_tau_modis,cosp_prs,time) t_index = 1 1
98756 2041284 ( 43674, 1, 1, 1) ( 1, 1, 1, 1) ( 45013, 2, 1, 1) ( 142, 1, 1, 1)
1968330 1.000000000000000E+02 0.000000000000000E+00 1.0E+02 1.000000000000000E+02 4.2E-02 1.600000000000000E+01
1968330 1.000000000000000E+02 0.000000000000000E+00 0.000000000000000E+00 0.000000000000000E+00
2041284 ( 43674, 1, 1, 1) ( 1, 1, 1, 1)
avg abs field values: 1.200910091400146E+00 rms diff: 4.7E+00 avg rel diff(npos): 4.2E-02
1.215296745300293E+00 avg decimal digits(ndif): 0.1 worst: 0.0
RMS CLMODIS 4.6744E+00 NORMALIZED 3.8692E+00

more details on https://my.cdash.org/viewTest.php?onlyfailed&buildid=1620355

@rljacob
Copy link
Member

rljacob commented Mar 12, 2019

So after fixing the build error you now have more diffs then before?

@rljacob
Copy link
Member

rljacob commented Mar 12, 2019

The large diffs are in COSP (satellite simulator) fields so they are diagnostic and don't change answers. Why would a land model change do that?

The FSDS diff is only in runs with the gnu compiler.

Only runs that have cam output show any diffs. Most tests only compare coupler history with baselines. No diffs there.

@thorntonpe
Copy link
Contributor Author

@rljacob , yes, I am also confused by this. I'm looking at the list of diffs, all are *MODIS fields - I don't know anything about how those are generated, and I'm not sure what the connection - if any - to the land model is. Who is the expert on these *MODIS fields? Is it possible these diffs are coming from another merged PR?

@brhillman
Copy link
Contributor

There was a bug fix for the MODIS simulator that was recently merged, and then reverted. Possible that the baseline and test case have inconsistent versions of this code change?

@rljacob
Copy link
Member

rljacob commented Mar 12, 2019

I don't see anything in the recent history of next involving MODIS.

@thorntonpe
Copy link
Contributor Author

One way or another, that seems a very likely culprit, given the diffs. I don't know enough about how the baselines are generated for testing on next to be able to track this down.

@rljacob
Copy link
Member

rljacob commented Mar 12, 2019

Ok if you go back a week PR #2770 caused MODIS changes and has been sitting on next since then. That's the reason.

@thorntonpe
Copy link
Contributor Author

So is there anything that I need to do for integration of this PR?

@jqyin
Copy link
Contributor

jqyin commented Mar 12, 2019

@rljacob I agree. The same DIFFs on prod_next starts on Mar. 7 https://my.cdash.org/index.php?project=ACME_Climate&date=2019-03-07. Looks like it's due to PR #2770. After the fix for debug mode F cases, it shows up now on integration next.

@jqyin jqyin merged commit 33ccf67 into master Mar 12, 2019
jqyin added a commit that referenced this pull request Mar 12, 2019
Implement ELM v2 data architecture for mass and energy state
and flux variables

Migrates mass and energy state and flux variables from CLM4.5
data structures, which mixed variables at multiple subgrid
levels, to ELM v2 data structures, which separate the variables
according to subgrid level. For example, carbon state variables
are moved from a single type into three distinct types, one for
gridcell-level, one for column-level, and one for vegetation
(patch)-level variables. Methods acting on these types are also
defined independently for each subgrid level. Migrates all energy,
water, carbon, nitrogen, and phosphorus state and flux variables
and data-type methods.

Leaves open the issue of how to structure subroutine calling
interfaces. The original code uses a variety of methods. Here, an
example is provided for the module biogeochem/CNCStateUpdate1Mod.F90
and all its upstream calls, based on explicit references to data
type instances at the clm_driver() level, and passed references at
all lower levels.

Branch tests bit-for-bit against the E3SM land developer test suite.

[BFB]
@jqyin
Copy link
Contributor

jqyin commented Mar 12, 2019

Merged #2742

rljacob pushed a commit that referenced this pull request Apr 12, 2021
updates for compatibility with XML to html tools
These are updates to cime based classes in support of the XML to HTML
auto-generation tools in https://github.com/NCAR/CESM_xml2html.
Also minor update to DOCN config_component.xml to correct invalid ASCII character.

Test suite: scripts_regression_tests.py B_CheckCode
Test baseline: N/A
Test namelist changes: N/A
Test status: bit for bit

Fixes [CIME Github issue #]

User interface changes?: None

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

Code review:jedwards
rljacob pushed a commit that referenced this pull request Apr 21, 2021
updates for compatibility with XML to html tools
These are updates to cime based classes in support of the XML to HTML
auto-generation tools in https://github.com/NCAR/CESM_xml2html.
Also minor update to DOCN config_component.xml to correct invalid ASCII character.

Test suite: scripts_regression_tests.py B_CheckCode
Test baseline: N/A
Test namelist changes: N/A
Test status: bit for bit

Fixes [CIME Github issue #]

User interface changes?: None

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

Code review:jedwards
@thorntonpe thorntonpe deleted the thorntonpe/lnd/archv2_cols branch March 22, 2023 19:59
bartgol pushed a commit that referenced this pull request Mar 27, 2024
…ng-add-logging

Automatically Merged using E3SM Pull Request AutoTester
PR Title: Make nudging log which file it's reading
PR Author: bartgol
PR LABELS: enhancement, I/O, AT: AUTOMERGE, nudging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB Land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants