APSIMInitiative / ApsimX

ApsimX is the next generation of APSIM
http://www.apsim.info
Other
130 stars 160 forks source link

Number of canopy layers is always 1 #3112

Closed BrianCollinss closed 5 years ago

BrianCollinss commented 5 years ago

State variables 'numNodes' and 'numLayers' are always 2 and 1, respectively. I see that in the private method 'DefineLayers' in MicroClimateZone.cs, 'numNodes' starts at 1 on all days. Is this intentional? Should the crop be considered as a single-layered canopy or I am missing some part of the theories?

BrianCollinss commented 5 years ago

@hol430 Hi Drew. By any chance, did you manage to have a look at this? I am working on a photosynthesis model based on a multi-layered canopy. But I am not sure if this works properly and I was not sure what happens if I change it as most of my commits fail to be built. And it seems very important to have the crop architecture simulated correctly. Cheers.

hol430 commented 5 years ago

When you say your code is failing to be built, do you mean the code in one of your pull requests, or the code on your local machine which you've written to address this issue?

BrianCollinss commented 5 years ago

I don't have any problem compiling the code on my Laptop. But most of the times my code cannot be compiled by Jenkins. You can see my last pull request, for instance. https://github.com/APSIMInitiative/ApsimX/pull/3102

hol430 commented 5 years ago

Yeah, there's been an issue with Jenkins over the last few weeks which has caused it to fail to compile sometimes. Not sure what caused it, and to be honest I don't think I've actually fixed it, but the last dozen or so builds don't seem to have been affected by it.

Either way, #3102 compiled fine, but it encountered some runtime errors in the soil nitrogen patchiness validation set located under ApsimX\Tests\Simulation\SoilNitrogenPatch\:

Growth and tissue turnover resulted in loss of mass balance for roots

If you run this file on your computer you should be able to reproduce the problem.

BrianCollinss commented 5 years ago

Thanks @hol430 . And do you have any idea why the number of canopy layers is kept at one?

hol430 commented 5 years ago

I have no idea why it's kept at 1, but I've sent @hut104 an email pointing him to this issue. He's muted all github notifications so unfortunately there's no way to get his attention via github.

BrianCollinss commented 5 years ago

@byzheng hi Bangyou. Do you have any comment on this issue?

hut104 commented 5 years ago

Hi, I think you may be referring to the code in ///

Break the combined Canopy into layers private void DefineLayers() ... Each day will have a different set of layers depending on the heights of each canopy. If the canopies are the same height you may only have one layer. If they are different heights you are likely to have 2-3 layers even if each canopy itself is a single layer due to differences in overlapping leaf layers. If canopies themselves have more than one layer then this is accentuated. numNodes simply starts at one because that's the start of the count. Nodes are the top or bottom of a layer, and so a single layered plant canopy will have 2 nodes (ie one top and one bottom) for 1 layer. You will have different numbers for other configurations.

BrianCollinss commented 5 years ago

Hi @hut104

Thanks for the response. And yes, that is the part of the code I was referring to.

I thought each canopy was supposed to have multiple layers. If this is not the case, please ignore what follows. And sorry if I am not making any sense as I am not aware of the idea behind the code.

Otherwise, there are couple of things:

double HeightMetres = Math.Round(Canopies[compNo].Canopy.Height, 5) / 1000.0; double DepthMetres = Math.Round(Canopies[compNo].Canopy.Depth, 5) / 1000.0; double canopyBase = HeightMetres - DepthMetres; if (Array.IndexOf(nodes, HeightMetres) == -1) { nodes[numNodes] = HeightMetres; numNodes = numNodes + 1; } if (Array.IndexOf(nodes, canopyBase) == -1) { nodes[numNodes] = canopyBase; numNodes = numNodes + 1; }

canopy layer

hut104 commented 5 years ago

The rounding is simply to avoid having thin layers of no consequence affecting the model (e.g. if you have two canopies that are 0.1mm different in height you would have a layer of that thickness at the top. So we use rounding to effectively set a lower limit for canopy layer thickness (and to avoid issues with comparing equality for floats). We start counting each day because numnodes is the number of canopy tops and bottoms. If you always have one canopy every day, and that canopy only has one layer (as most crop models do) then you will always end up with two nodes (top and bottom) for one layer. If you are looking to have a multi-layered canopy, we would need to pass that information to micromet via some sort of more complex type. We haven't implemented this in NextGen because till now no crop has sought to have layered canopies.

BrianCollinss commented 5 years ago

Thanks a lot @hut104 . Quite informative.