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

fates-interface: integration of external model #1536

Merged
merged 3 commits into from
May 26, 2017

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented May 14, 2017

This change set completes a first version of 1) the ALM-side interface for FATES,
2) initializes FATES as a submodule and 3) adds a test to acme_land_developer.
Restart simulations, while having the code in place, are not passing tests yet and
will be addressed in forthcoming changesets. Modifications to the namelist build
system were necessary. Thus, there are trivial conflicts with namelists generated
by master, specifically use_ed=.false. is now appended to all namelists where it
is not used.This has no impact on existing tests' model output, however CLM
regression tests will fail namelist comparisons for this reason.
As this PR initializes a new submodule, all CLM users must update their submodule
pointers to bring the fates codeset in, without this the code will not compile.

[BFB]
[NML]

@rgknox rgknox added BFB PR leaves answers BFB Land labels May 14, 2017
@rgknox rgknox requested review from rljacob, bishtgautam and jqyin May 14, 2017 23:26
@rgknox
Copy link
Contributor Author

rgknox commented May 14, 2017

This PR replaces #1531.

@rljacob
Copy link
Member

rljacob commented May 15, 2017

Does this depend on #1460

@bishtgautam
Copy link
Contributor

@rljacob: This PR doesn't depend on #1460

Copy link
Contributor

@bishtgautam bishtgautam left a comment

Choose a reason for hiding this comment

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

Few minor changes

# ED is not a clm4_0 option and should not be used with crop and not with clm4_0
fatal_error("** Cannot turn betr mode on with crop or with clm4_0 physics.\n" );
}
# if ( $nl_flags->{'ed_mode'} == 1 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • The original logic in the if-loop was incorrect and should be fixed by
    if ( $nl_flags->{$var} == 1 ) {
       # BeTR is not a clm4_0 option and should not be used with crop and not with clm4_0
       fatal_error("** Cannot turn betr mode on with crop or with clm4_0 physics.\n" );
    }

@@ -111,13 +111,11 @@ module clm_instMod
type(phosphorusflux_type) :: phosphorusflux_vars
type(clm_bgc_interface_data_type) :: clm_bgc_data
type(chemstate_type) :: chemstate_vars
! (FATES-INTERF)
! type(hlm_fates_interface_type) :: clm_fates (FATES-INTERF)
type(hlm_fates_interface_type) :: clm_fates
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename clm_fates to alm_fates.

@@ -655,8 +661,6 @@ contains

end subroutine ncd_inqvdlen_byDesc


!-----------------------------------------------------------------------
subroutine ncd_inqvdlen_byName(ncid,varname,dimnum,dlen,err_code)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add back the two deleted lines:

	
  !-----------------------------------------------------------------------

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do, I think those got deleted in the rebasing effort, there were some conflicts in there if I remember correctly, and the way the head vs base diffs were structured was a little confusing.

@bishtgautam
Copy link
Contributor

@rgknox: Following changes are needed to the first commit message:

  • Add [BFB] and [NML] at the end of the message, and
  • Break the commit message into multiple lines instead of a single
    long line because that message is copied & pasted in the merge
    commit.

Copy link
Contributor

@jqyin jqyin left a comment

Choose a reason for hiding this comment

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

Looks good. The threading is in initialization part and not nested threading in the main driver calls.

bishtgautam pushed a commit that referenced this pull request May 18, 2017
…t (PR #1536)

This change set completes a first version of 1) the ALM-side interface for FATES,
2) initializes FATES as a submodule and 3) adds a test to acme_land_developer.
Restart simulations, while having the code in place, are not passing tests yet and
will be addressed in forthcoming changesets. Modifications to the namelist build
system were necessary. Thus, there are trivial conflicts with namelists generated
by master, specifically use_ed=.false. is now appended to all namelists where it
is not used.This has no impact on existing tests' model output, however CLM
regression tests will fail namelist comparisons for this reason.
As this PR initializes a new submodule, all CLM users must update their submodule
pointers to bring the fates codeset in, without this the code will not compile.

[BFB]
[NML]
@bishtgautam
Copy link
Contributor

@amametjanov reported following error on Cetus for SMS_Lm1.f45_f45.ICLM45ED.cetus_ibm.clm-fates

4: "/gpfs/mira-home/azamatm/repos/ACME-test/components/clm/src/external_models/fates/biogeochem/EDCohortDynamicsMod.F90", line 837: 1525-109 Error encountered while attempting to deallocate a data object.  The program will stop.
2017-05-19 16:30:32.081 (WARN ) [0xfff8264ca70] CET-00000-11331-128:2129189:ibm.runjob.client.Job: normal termination with status 1 from rank 4```

@rgknox: Do you have access to Cetus to debug this failure?

bishtgautam pushed a commit that referenced this pull request May 19, 2017
This change set completes a first version of 1) the ALM-side interface for FATES,
2) initializes FATES as a submodule and 3) adds a test to acme_land_developer.
Restart simulations, while having the code in place, are not passing tests yet and
will be addressed in forthcoming changesets. Modifications to the namelist build
system were necessary. Thus, there are trivial conflicts with namelists generated
by master, specifically use_ed=.false. is now appended to all namelists where it
is not used.This has no impact on existing tests' model output, however CLM
regression tests will fail namelist comparisons for this reason.
As this PR initializes a new submodule, all CLM users must update their submodule
pointers to bring the fates codeset in, without this the code will not compile.

[BFB]
[NML]
@bishtgautam
Copy link
Contributor

The duration of the ICLM45ED test has been decreased to avoid failure on Cetus.
@rgknox has requested an account for ALCC to debug this failure in coming days.
This updated PR has been re-merged to next.

@rgknox rgknox force-pushed the rgknox/lnd/fates-part3 branch 2 times, most recently from 82d5c18 to 1ee1d0a Compare May 23, 2017 20:50
bishtgautam pushed a commit that referenced this pull request May 23, 2017
bishtgautam pushed a commit that referenced this pull request May 23, 2017
…into next (PR #1536)"

This reverts commit 16bb80b, reversing
changes made to db6993d.
bishtgautam pushed a commit that referenced this pull request May 23, 2017
This change set completes a first version of 1) the ALM-side interface for FATES,
2) initializes FATES as a submodule and 3) adds a test to acme_land_developer.
Restart simulations, while having the code in place, are not passing tests yet and
will be addressed in forthcoming changesets. Modifications to the namelist build
system were necessary. Thus, there are trivial conflicts with namelists generated
by master, specifically use_ed=.false. is now appended to all namelists where it
is not used.This has no impact on existing tests' model output, however CLM
regression tests will fail namelist comparisons for this reason.
As this PR initializes a new submodule, all CLM users must update their submodule
pointers to bring the fates codeset in, without this the code will not compile.

[BFB]
[NML]
Copy link
Member

@rljacob rljacob left a comment

Choose a reason for hiding this comment

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

Should have reviewed this sooner. All the changes to files in cime need to be in a separate commit. This is because cime is an external.

bishtgautam pushed a commit that referenced this pull request May 24, 2017
This reverts commit 9233c4b, reversing
changes made to 1793e10.
@rgknox
Copy link
Contributor Author

rgknox commented May 24, 2017

ok, I should be able to do an interactive rebase, and then go into edit mode to split this up.

Or, I guess I can just git reset HEAD~ and go from there.

@rgknox rgknox force-pushed the rgknox/lnd/fates-part3 branch from 1ee1d0a to 86c72aa Compare May 24, 2017 03:18
@rgknox
Copy link
Contributor Author

rgknox commented May 24, 2017

Commits have been split into 3 parts. The second two are cime side, the first is components ie non-cime.

For comparison I copied and renamed the single commit branch here:

https://github.com/rgknox/ACME/tree/rgknox/lnd/fates-part3-pre-split

@rljacob
Copy link
Member

rljacob commented May 24, 2017

Looks good. Gautum, you can re-merge this to next.

bishtgautam pushed a commit that referenced this pull request May 24, 2017
This change set completes a first version of 1) the ALM-side interface for FATES,
2) initializes FATES as a submodule and 3) adds a test to acme_land_developer.
Restart simulations, while having the code in place, are not passing tests yet and
will be addressed in forthcoming changesets. Modifications to the namelist build
system were necessary. Thus, there are trivial conflicts with namelists generated
by master, specifically use_ed=.false. is now appended to all namelists where it
is not used.This has no impact on existing tests' model output, however CLM
regression tests will fail namelist comparisons for this reason.
As this PR initializes a new submodule, all CLM users must update their submodule
pointers to bring the fates codeset in, without this the code will not compile.

[BFB]
[NML]
@bishtgautam bishtgautam merged commit 86c72aa into E3SM-Project:master May 26, 2017
bishtgautam pushed a commit that referenced this pull request May 26, 2017
This change set completes a first version of 1) the ALM-side interface for FATES,
2) initializes FATES as a submodule and 3) adds a test to acme_land_developer.
Restart simulations, while having the code in place, are not passing tests yet and
will be addressed in forthcoming changesets. Modifications to the namelist build
system were necessary. Thus, there are trivial conflicts with namelists generated
by master, specifically use_ed=.false. is now appended to all namelists where it
is not used.This has no impact on existing tests' model output, however CLM
regression tests will fail namelist comparisons for this reason.
As this PR initializes a new submodule, all CLM users must update their submodule
pointers to bring the fates codeset in, without this the code will not compile.

[BFB]
[NML]
jgfouca pushed a commit that referenced this pull request Jun 2, 2017
This change set completes a first version of 1) the ALM-side interface for FATES,
2) initializes FATES as a submodule and 3) adds a test to acme_land_developer.
Restart simulations, while having the code in place, are not passing tests yet and
will be addressed in forthcoming changesets. Modifications to the namelist build
system were necessary. Thus, there are trivial conflicts with namelists generated
by master, specifically use_ed=.false. is now appended to all namelists where it
is not used.This has no impact on existing tests' model output, however CLM
regression tests will fail namelist comparisons for this reason.
As this PR initializes a new submodule, all CLM users must update their submodule
pointers to bring the fates codeset in, without this the code will not compile.

[BFB]
[NML]
jgfouca pushed a commit that referenced this pull request Feb 27, 2018
This change set completes a first version of 1) the ALM-side interface for FATES,
2) initializes FATES as a submodule and 3) adds a test to acme_land_developer.
Restart simulations, while having the code in place, are not passing tests yet and
will be addressed in forthcoming changesets. Modifications to the namelist build
system were necessary. Thus, there are trivial conflicts with namelists generated
by master, specifically use_ed=.false. is now appended to all namelists where it
is not used.This has no impact on existing tests' model output, however CLM
regression tests will fail namelist comparisons for this reason.
As this PR initializes a new submodule, all CLM users must update their submodule
pointers to bring the fates codeset in, without this the code will not compile.

[BFB]
[NML]
jgfouca pushed a commit that referenced this pull request Mar 14, 2018
This change set completes a first version of 1) the ALM-side interface for FATES,
2) initializes FATES as a submodule and 3) adds a test to acme_land_developer.
Restart simulations, while having the code in place, are not passing tests yet and
will be addressed in forthcoming changesets. Modifications to the namelist build
system were necessary. Thus, there are trivial conflicts with namelists generated
by master, specifically use_ed=.false. is now appended to all namelists where it
is not used.This has no impact on existing tests' model output, however CLM
regression tests will fail namelist comparisons for this reason.
As this PR initializes a new submodule, all CLM users must update their submodule
pointers to bring the fates codeset in, without this the code will not compile.

[BFB]
[NML]
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.

4 participants