Enable maxpft>1 #161

niklaswr · 2023-02-24T12:04:18Z

set maxpft=4 as new default in bldsva/intf_oas3/common_build_interface.ksh
rewrite workaround in bldsva/intf_oas3/common_build_interface.ksh correcting forc_hgt according to pft types present in CLM35 grid cell. This is now working for maxpft!=1 as well.

jjokella · 2023-02-27T07:57:27Z

From the PDAF-side I do not have any objections to this.

I have one question: Are currently used CLM3.5 models (most prominently the FallSchoolCase) expected to behave differently after this PR? Then, I would have to update the test routines.

niklaswr · 2023-02-27T08:13:33Z

From the PDAF-side I do not have any objections to this.

I have one question: Are currently used CLM3.5 models (most prominently the FallSchoolCase) expected to behave differently after this PR? Then, I would have to update the test routines.

The current behaviour will change, yes.
maxpft is hardcoded in bldsva/intf_oas3/common_build_interface.ksh, which I changed from maxpft=1 to maxpft=4 with this PR.
Stefan P. also mentioned this and suggested implementing a command line argument to control maxpft. This way, the default could stay at maxpft=1, but the user could change it.
However, I do not have a clear opinion on this yet. As far as I can see, the CLM3.5 default is maxpft=4, and maxpft=1 is kind of unexpected. So setting the TSMP default to maxpft=1 is not intuitive to me.

jjokella · 2023-02-27T13:34:38Z

As far as I can see, the CLM3.5 default is maxpft=4, and maxpft=1 is kind of unexpected. So setting the TSMP default to maxpft=1 is not intuitive to me.

Ah, I see. I agree that the default should be aligned with CLM-defaults as far as possible. The only problem is that we have to deal with the backward compatibility of results inside the TSMP-framework. Maybe something to discuss in a meetings with supervisors.

niklaswr · 2023-02-27T13:50:13Z

Hmm, backward compatibility is a topic I do not agree / I do not understand.
Why should we aim for backward compatibility?
We do not break any workflow with a new commits, as people can checkout old commits any time. They even should specific the used TSMP commit within their workflow for reasons of reproducibility.
So to my opinion backward compatibility should not hold us back if we are going to change something towards the 'right' way (which has to be defined, for sure :) ). Especially as we are to few developers to keep stuff real backward compatible

However, I totally agree in providing some consistency that people do not have to upgrade their workflows with any new commit.
And I suggest to make user of a proper tag system for TSMP. This would make things more clear and people can use specific tags for their workflows - most probably avoiding some conflicts like 'this commit breaks my workflow'.
Anyhow, this might be a more general topic as for this PR :)

DCaviedesV · 2023-02-27T16:34:07Z

I think the issue of backward compatibility is a tricky one. Things should not change too dramatically/often that very large changes in workflows are warranted.
On the other hand, @niklaswr is right. Users should be able to document what they are doing, with model versions, and indeed with the specific commits they are working, so that they can have consistent experiments, or at least check divergences in inconsistent experiments against the commit logs. I understand @jjokella concern, since there is no guarantee that users do this, but that is a bad practice. If that is the case, they have worse problems :)

On the core of the subject, does anyone know why maxpft=1 was set, given that it does not match the CLM3.5 default? Is the implication that additional FPTs specified in CLM are simply ignored?

s-poll · 2023-02-27T18:38:18Z

TSMP is designed to use the mosaic approach, which is known to be superior of the tile approach (see e.g. Ament and Simmer 2006; https://doi.org/10.1007/s10546-006-9066-4). In the mosaic approach one keep the location information of the land surface (finer land surface resolution) compared to the tile approach, where the different PFTs within one grid points are bundled. However, using one of the mentioned approaches outperform using none.

As @niklaswr already mentioned I would either opt to add a control switch in build_tsmp.ksh for maxpft and/or to check if the simulation results of CLM are (binar) identical, if one performs a simulation with 100% of one pft for each grid point with maxpft=1 and =4. Maybe it would be good to check the second part anyhow.

niklaswr · 2023-02-28T08:36:03Z

Okay, summarising the above comments:

We all agree that enabling the option to use maxpft>=1 should be implemented.
We all agree that a command line option to change maxcpft should be implemented.
The default value of maxpft is TBD. Either maxpft=1 as before, or maxpft=4 as the CLM3.5 default.

I will provide an update to this PR that implements a command line option for maxpft.
In the meantime, how should we decide on the default value? Just vote? :)

I also see the unresolved issue of testing the impact of this PR, also mentioned in @s-poll's last comment.
I have already done three 24h simulations based on this change. i) maxpft=1, ii) maxpft=2 and iii) maxpft=4. Simulation results can be found here:
/p/scratch/cesmtst/wagner6/FZJ-IBG3_Climatrun-Template_TestMaxPft/simres/SPINUP.
The simulations run fine and you can clearly see the effect of changing maxpft.
However, I have no idea how to draw a conclusion in terms of test passed, we can use the new version.
Does anyone have an idea?

Just for the sake of completeness, Im mentioning @skollet here.

s-poll · 2023-03-03T08:46:06Z

Hi Niklas,
Thank you for the summary and for implementing the maxpft option! Very useful.

Could you please test if the simulation results change in case of modifying maxpft for a setup, where PFT_PCT is always set to 100% for on PFT-Type (e.g. in the nrw test case)? Thus, we would be able to make a educated discussion on the maxpft default.

niklaswr · 2023-03-06T15:18:16Z

@Stefan Poll, I did the test you mentioned and a little bit more.

I run the following simulations:

a) 
maxpft  : 4
src-code: patched

b) 
maxpft  : 1 
src-code: patched

c) 
maxpft  : 1
src-code: master

src-code: patched means I used the changed function, which is correcting the forc_hgt, and thus the src-code of this PR.
src-code: master means I used the src-code from master branch, and thus without any changes.
All simulations using the same static files with only one PFT per grid cell (PCT_PFT=100 for related pft)

The results are:
i) a) minus b) = 0 --> no impact of maxpft if a single pft is present only.
but
ii) a) minus c) != 0
iii) b) minus c) !=0
ii) minus iii) = 0
So the new function does change the model results compared, what I did not saw before and which I need to investigate further.

s-poll · 2023-03-07T14:37:33Z

Thank you for the detailed check! This is indeed an unexpected behavior (especially for case iii). Please keep us up to date. I am available, if you want to talk bilateral about this PR.

niklaswr · 2023-03-15T11:07:04Z

To study the problem mentioned above, that rewriting the function updating forcing_hgt changes the model results, I run the following test:

First, I printed (into a file) all the values that are part of the corresponding code section, for the two cases b) and c) from above.
Comparing the printed tables shows that the new codes do not change the forcing_hgt in general, but some values are different for the 14th or 15th floating point digit, most probably due to machine inaccuracy.
However, as this was the only difference I found, I run another test.

Second, I run two simulations with the same setup and the same model. The difference between these two simulations is that
i) using a fixed hardcoded forcing_hgt for all pixels of forcing_hgt = 20.0000000000000000
ii) using a fixed hardcoded forcing_hgt for all pixels of forcing_hgt = 20.0000000000000059
After 24h of simulation, the model results show differences comparable to the difference observed above. So a change in forcing_hgt of the order of 5.9E-15 leads to noticeable changes after 24h of simulation.
More generally, this test shows that even changes of the order of the machine inaccuracy does quickly change the model results due to the chaotic behaviour of the Earth-System.

So my conclusion for this PR is, that the observed differences can be explained by machine inaccuracy and are not the result of a systematic error in the rewritten function updating forcing_hgt.
Since the printed tables of forcing_hgt (CellCorrectionforchgt_github_t00h.txt, CellCorrectionforchgt_niklaswr_t00h.txt) does show that the rewritten function is working, I do ask again:

Do we need any further testing?
Are there any other concerns about accepting this PR?

- set `maxpft=4` as new default in `bldsva/intf_oas3/common_build_interface.ksh` - rewrite workaround in `bldsva/intf_oas3/common_build_interface.ksh` correcting `forc_hgt` according to pft types present in CLM35 grid cell. This is now working for `maxpft!=1` as well.

- Add a command line option to modify maxpft long: --maxpft=X short -f=X - Default behaviour is still maxpft=1

- I have marked my changes in the src-code with !NWa to indicate which change was introduced by me. However, this is what git is for, so it is not necessary. As a good practice, I have removed these comments.

s-poll

Without having it explicitly tested, changes look fine.

bldsva/intf_oas3/clm3_5/tsmp/Biogeophysics1Mod.F90

bldsva/build_tsmp.ksh

s-poll · 2023-03-27T14:51:26Z

bldsva/intf_oas3/clm3_5/tsmp/Biogeophysics1Mod.F90

    do pi = 1,max_pft_per_col
+      ! num_nolakec: number of column non-lake points in column filter
      do fc = 1,num_nolakec
        c = filter_nolakec(fc)
        l = clandunit(c)
        g = cgridcell(c)
        if (pi <= npfts(c)) then


could you explain this line to me, thanks.

No, actually I cannot. This is the official 'description' of this variable taken from Biogeophysics1Mod.F90#L80.

bldsva/intf_oas3/clm3_5/tsmp/Biogeophysics1Mod.F90

- Adding best practice for FORTRAN array notation. Scalar: A vs. Vector: A(:) Co-authored-by: s-poll <[email protected]>

- add comment indicating maxpft=4 is CLM3.5 default. Co-authored-by: s-poll <[email protected]>

niklaswr · 2023-03-28T05:42:39Z

@DCaviedesV
Once the open conversation with @s-poll is resolved, this PR is ready to merge.

Right now this PR aims to merged into the master. Should I change this to merge with the release_candidate instead?

DCaviedesV · 2023-03-28T06:21:17Z

@niklaswr, yes, let's collect this PR with release_candidate. I'll take note of this for the changelog.

bldsva/intf_oas3/clm3_5/tsmp/Biogeophysics1Mod.F90

s-poll

Changes fine. PR could be merged from my side.

- Remove unused src-code Co-authored-by: s-poll <[email protected]>

niklaswr added enhancement New feature or request CLM Issues regarding CLM component or its coupling labels Feb 24, 2023

niklaswr requested review from jjokella and s-poll February 24, 2023 12:04

niklaswr self-assigned this Feb 24, 2023

niklaswr linked an issue Feb 24, 2023 that may be closed by this pull request

Enable CLM3.5 land cover tile approach #158

Closed

1 task

niklaswr removed the request for review from jjokella March 2, 2023 09:08

niklaswr marked this pull request as draft March 6, 2023 15:18

niklaswr marked this pull request as ready for review March 27, 2023 07:15

niklaswr force-pushed the CLM35TileApproach branch from f237ab6 to 0bd4844 Compare March 27, 2023 09:07

niklaswr added 3 commits March 27, 2023 11:10

Enable maxpft>1

cddef6e

- set `maxpft=4` as new default in `bldsva/intf_oas3/common_build_interface.ksh` - rewrite workaround in `bldsva/intf_oas3/common_build_interface.ksh` correcting `forc_hgt` according to pft types present in CLM35 grid cell. This is now working for `maxpft!=1` as well.

Add a command line option to modify maxpft

f79cf56

- Add a command line option to modify maxpft long: --maxpft=X short -f=X - Default behaviour is still maxpft=1

Remove personal comments

c6afa9c

- I have marked my changes in the src-code with !NWa to indicate which change was introduced by me. However, this is what git is for, so it is not necessary. As a good practice, I have removed these comments.

niklaswr force-pushed the CLM35TileApproach branch from 0bd4844 to c6afa9c Compare March 27, 2023 09:10

s-poll reviewed Mar 27, 2023

View reviewed changes

niklaswr and others added 2 commits March 28, 2023 07:24

Update bldsva/intf_oas3/clm3_5/tsmp/Biogeophysics1Mod.F90

a9e6324

- Adding best practice for FORTRAN array notation. Scalar: A vs. Vector: A(:) Co-authored-by: s-poll <[email protected]>

Update bldsva/build_tsmp.ksh

670c6c0

- add comment indicating maxpft=4 is CLM3.5 default. Co-authored-by: s-poll <[email protected]>

niklaswr changed the base branch from master to release_candidate March 28, 2023 06:49

s-poll reviewed Mar 28, 2023

View reviewed changes

bldsva/intf_oas3/clm3_5/tsmp/Biogeophysics1Mod.F90 Outdated Show resolved Hide resolved

s-poll approved these changes Mar 28, 2023

View reviewed changes

Update bldsva/intf_oas3/clm3_5/tsmp/Biogeophysics1Mod.F90

85359a2

- Remove unused src-code Co-authored-by: s-poll <[email protected]>

niklaswr merged commit cbca8ec into HPSCTerrSys:release_candidate Mar 28, 2023

niklaswr deleted the CLM35TileApproach branch March 28, 2023 11:16

s-poll mentioned this pull request Apr 6, 2023

Enable CLM3.5 land cover tile approach #158

Closed

1 task

chartick mentioned this pull request Apr 18, 2023

Setting the release candidate as the new master #187

Merged

s-poll mentioned this pull request Sep 8, 2023

MaxPFT handling in CLM3.5 coupled vs sta #212

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable maxpft>1 #161

Enable maxpft>1 #161

niklaswr commented Feb 24, 2023

jjokella commented Feb 27, 2023

niklaswr commented Feb 27, 2023

jjokella commented Feb 27, 2023

niklaswr commented Feb 27, 2023

DCaviedesV commented Feb 27, 2023

s-poll commented Feb 27, 2023 •

edited

Loading

niklaswr commented Feb 28, 2023 •

edited

Loading

s-poll commented Mar 3, 2023

niklaswr commented Mar 6, 2023 •

edited

Loading

s-poll commented Mar 7, 2023

niklaswr commented Mar 15, 2023 •

edited

Loading

s-poll left a comment

s-poll Mar 27, 2023

niklaswr Mar 28, 2023

niklaswr commented Mar 28, 2023

DCaviedesV commented Mar 28, 2023

s-poll left a comment

Enable maxpft>1 #161

Enable maxpft>1 #161

Conversation

niklaswr commented Feb 24, 2023

jjokella commented Feb 27, 2023

niklaswr commented Feb 27, 2023

jjokella commented Feb 27, 2023

niklaswr commented Feb 27, 2023

DCaviedesV commented Feb 27, 2023

s-poll commented Feb 27, 2023 • edited Loading

niklaswr commented Feb 28, 2023 • edited Loading

s-poll commented Mar 3, 2023

niklaswr commented Mar 6, 2023 • edited Loading

s-poll commented Mar 7, 2023

niklaswr commented Mar 15, 2023 • edited Loading

s-poll left a comment

Choose a reason for hiding this comment

s-poll Mar 27, 2023

Choose a reason for hiding this comment

niklaswr Mar 28, 2023

Choose a reason for hiding this comment

niklaswr commented Mar 28, 2023

DCaviedesV commented Mar 28, 2023

s-poll left a comment

Choose a reason for hiding this comment

s-poll commented Feb 27, 2023 •

edited

Loading

niklaswr commented Feb 28, 2023 •

edited

Loading

niklaswr commented Mar 6, 2023 •

edited

Loading

niklaswr commented Mar 15, 2023 •

edited

Loading