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

First pass at adding POP namelist #476

Draft
wants to merge 3 commits into
base: CABLE-POP_TRENDY
Choose a base branch
from

Conversation

Whyborn
Copy link

@Whyborn Whyborn commented Nov 17, 2024

CABLE

Thank you for submitting a pull request to the CABLE Project.

Description

@juergenknauer @har917

Making the POP parameters configurable via namelist. At the moment, I've made everything bar PI and EPS changable via the namelist, but discussions with @juergenknauer suggest that this probably isn't wise. Opening this PR now to prompt discussions. Some things that have been discussed:

  • Some declared variables are unused: Q, N_EXTENT, NAGE_MAX, NPATCH1D and TIMEBASE_FACTOR. Will be removed unless good arguments can be made otherwise.
  • NLAYERS should be restricted to 1. Currently there is machinery for including multiple layers (although I can't find what these "layers" represent in Vanessa's original publication), but I think all layers will contain identical data. Will this be extended in future?
  • How does the existing configuration interact with POPLUC and BLAZE? What do we need to be careful of? It looks to me like it's quite self-contained, with the variables coming in from the "main" CABLE being veg%disturbance_frequency and veg%disturbance_intensity, which I assume may also be set via BLAZE?

Also, please ping anyone else that would have input on this that I haven't mentioned.

Type of change

Please delete options that are not relevant.

  • [] Enhancement

Further discussions to come around exactly which variables should be changeable by the user.
@Whyborn Whyborn self-assigned this Nov 17, 2024
@har917
Copy link
Collaborator

har917 commented Nov 17, 2024

@juergenknauer @AlisonBennett @Whyborn My general position is that we should only extract options/parameters etc. that we envisage that we will want to assess/vary - extracting everything creates a risk for unexperienced users/typos etc.. I'd follow @juergenknauer view on which bits should be in/out.

We would have to check but I don't think there is much (if any) intersection between the POP-parameters and POPLUC and BLAZE from a technical sense.

My other comment on this is that I think (and have never understood why this wasn't the case) that veg%disturbance_frequency and veg%disturbance_intensity should be part of the POP type - not the VEG type. Other than as a means to pass BLAZE information back to POP they are not used elsewhere in the model. Passing around via veg% acts as an impediment to nice modularisation.

@Whyborn
Copy link
Author

Whyborn commented Nov 17, 2024

Fair enough. I personally like having more things configurable than not, with the very important caveat that the values have reasonable defaults to fall back to if they are not set by the user and documentation on what range of values is reasonable.

I'm assuming that it's precisely because BLAZE will want to modify the disturbance parameters that they were tacked onto the veg parameter. Wouldn't surprise me if they were POP_Constants parameters in the past, and were moved to veg when BLAZE came about. Does BLAZE do anything without POP? Should I be thinking of BLAZE as an augmentation to CABLE (i.e. has affects across a few science modules including POP), or an augmentation to POP specifically?

@har917
Copy link
Collaborator

har917 commented Nov 17, 2024

BLAE/SIMFIRE was originally written developed as a stand-alone module (and has been implemented in other models).

Our incarnation of BLAZE however is now so tied into POP that it should be viewed as an augmentation of POP (not of CABLE-CASA).

I suspect that it was originally envisaged that the veg% variables would be PFT/biome dependent and be provided to the model via one of the other input routes alongside the other PFT parameters - but it first went very simple (universal parameters) then a lot more complex (linking in with BLAZE).

@juergenknauer
Copy link
Collaborator

@Whyborn and I have discussed this via email and I have suggested a list of parameters that could be prioritised for the namelist. I agree that the two parameters currently in %veg (disturbance_interval and disturbance_intensity) would also be best located in the POP namelist. But we weren't entirely sure if they are used somewhere else in the POP = FALSE case.

@har917
Copy link
Collaborator

har917 commented Nov 18, 2024

I can see mentions of disturbance_interval, disturbance_frequency and disturbance_intensity in various places - casa_inout, casa_cable, casa_variable, POP code, cable_define_types, BLAZE, cable_parameters, MPI code and the version controlled parameter files.

I can't see any use of these disturbance variables outside of POP - and the only non-trivial evaluation coming from BLAZE. As a result I think a tidy up is plausible but suspect that this is not part of this process (but is likely part of CABLE4)

@Whyborn
Copy link
Author

Whyborn commented Nov 19, 2024

I came to a similar conclusion- said disturbance variables are initialised (and more dangerously output) in various places, but don't seem to be used for anything meaningful outside POP.F90 and POPLUC.F90. I'm going to leave most of them as is, and for now interact with veg%disturbance_interval and veg%disturbance_intensity and ensure that the local copy of disturbance_interval in POPLUC.F90 remains the same as veg%disturbance_interval according to this comment.

@Whyborn
Copy link
Author

Whyborn commented Nov 20, 2024

This turns out to be a more significant piece of work than I expected- there are many operations in the code that only work because array sizes are known at compile time, which is no longer the case when the options become configurable.

@Whyborn
Copy link
Author

Whyborn commented Nov 21, 2024

@har917 @juergenknauer @ccarouge On the issues mentioned at the meeting- there is an operation used in many, many places throughout the code that is only allowed when the array sizes are known at compile time. This is the action of retrieving attributes from a list of derived types. For context, the derived type structure is:

  • POP%POP_grid contains all the grid cells in the simulation.
  • Each grid cell contains a set of base type attributes (e.g. REAL, INTEGER) and an array of patches.
  • Each patch contains a set of base type attributes and an (currently size 1) array of layers.
  • Each layer contains base type attributes and an array of cohorts.
  • Cohorts only contain base type attributes i.e. are the lowest level derived type.

If the array size of these derived types is known at compile time, then the compiler can infer that when you ask for patch%biomass for example, it knows that you want an array of biomasses built by taking the biomass from each patch. When the arrays are not a fixed size, this is no longer allowed. This means operations like this are no longer allowed:

age_min = MAXVAL(pop%pop_grid(g)%patch(:)%age(1), &
    pop%pop_grid(g)%patch(:)%age(1).LE.age(iage))

The patch(:)%age(1) is no longer a valid operation, a manual looping over the indices of patch is required. This operation is very common.

@Whyborn
Copy link
Author

Whyborn commented Nov 21, 2024

There's also the annoyance that everywhere assumes that patch(i)%layer is an array rather than a singleton, which seems to be a likely change given discussions with Juergen around NLAYER.

@Whyborn
Copy link
Author

Whyborn commented Nov 25, 2024

I think I will need to test this in stages. Discussion with Juergen indicate that we are unlikely to ever use multiple layers in the POP setup, so we should change the layer type in the patch to be a singleton rather than an array. Unfortunately, this changes the array shape of many things written to the output downstream, hampering bitwise compatibility tests.

I will make all the changes around looping through the patch properties, while leaving patch%layer as a length 1 array to allow direct comparisons. Once we are satisfied with these comparisons, then I go through the process of changing layer to a singleton.

@Whyborn
Copy link
Author

Whyborn commented Dec 12, 2024

Ben Smith suggested that NLAYER has a reasonable chance to be developed in the future, especially after it gets merged into main and gets used by more people. Based on this, I think I will treat NLAYER as a "tunable" parameter that is set to 1 internally for now, to make it easier for future development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants