Open hol430 opened 4 years ago
@jbrider I've had a quick look into why EP is negative. It appears that we expect the variable dltSwDep to be negative. When using soilwat, it's calculated like this:
dltSwDep[layer] = -1 * divide(supply[layer], totalSupply, 0.0) * swDemand;
When using swim we just fetch it via the science API:
scienceAPI.get("uptake_water_maize", "",1, dltSwDep);
But it comes in as positive, and we don't multiply it by -1. When calculating other variables like uptake and EP, we always multiply dltSwDep by -1, which means that when we're using swim, EP and uptake end up negative. It seems like the obvious fix here would be to just multiply the incoming dltDwDep by -1 when using swim, so that all other calculations can remain the same:
scienceAPI.set("dlt_sw_dep", "mm", dltSwDep);
for(unsigned i=0;i < dltSwDep.size();i++)
dltSwDep[i] *= -1;
quick fix - sounds great. hopefully that fixes the higher soil moisture as well...
Looks like there is no code in sorghum or MaizePioneer to even check if swim is present. Do we want them to basically do what maize does?
Yes - hopefully that should be straight forward?
Yep, should be. I will create a new issue for sorghum/Maizepioneer so that I can resolve the issue and get an installer out.
@hol430 - following up - was there a new issue created for this missing feature in sorghum? I can't find it..
I just had a poke around and it looks like I never ended up fixing it. I had a quick go at this, but am getting build errors when I try to rebuild apsim (to my huge surprise :), so I'm unable to test my fix. For what it's worth, I've attached my change to this issue. If you guys think it's sensible, I can make a PR (or you can do it if you want). There were one or two bits I wasn't sure of...the jankiness index of this patch is rather high.
Strangely enough, 15mins of diffing around gave me much the same patch. I'll make a PR and look for some damage. Ta.
The following issues have been observed in simulations with the 7.10 maize model when SWIM is present:
The current workaround is to use the maize model from 7.9 but it would be good if this wasn't necessary.