search

APSIMInitiative / ApsimX

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

Problem entering new initial soil water % #7562

Closed JodyBig closed 2 years ago

JodyBig commented 2 years ago

When entering numbers into the initial soil water % field random numbers appear. This behaviour is only on some soils but is repeatable. See the Site permutation = Red Kandosol (Berrigan No537-YP) in the attached example. ExperimentBased_Scenarios.zip

zur003 commented 2 years ago

@hol353 - I'd like your input on this, as it is in part an architectural problem. The problem is arising in Water.cs (line 207), where it locates the "Physical" node using: public IPhysical Physical => FindInScope<IPhysical>(); The problem is that in this case there are multiple soil models "in scope", and it is the "first" that gets returned, which does not necessarily correspond with the current soil. This causes havoc when MapConcentration is called, as it wrongly tries to map two soil profiles that don't correspond.

Several options that I can see:

  1. Use "FindSibling" rather than "FindInScope" when locating IPhysical and ISoilWater. This will using return the "right" node, but is it always safe to assume that these models will in fact be siblings?
  2. Try "FindSibling" first, and if that fails, fall back to using "FindInScope".
  3. Develop logic in Model.cs to implement a "FindNearest" method, so that "child" or "sibling" nodes are preferenced over "cousin" nodes. The problem here might be in deciding how "nearness" should be defined. It also would probably be a bit slower to execute.

There are several open Issues dealing with soil water at the moment. This might be the underlying problem for several of them. I think it's a reasonably serious bug.

hol430 commented 2 years ago

That is interesting. What is the context here that results in multiple soils being in scope?

One option for locating the physical node might be to try something like FindAncestor<Soil>().FindDescendant<Physical>(), though this will obviously still fail if the water node is not under a soil (e.g. if it's under replacements - but I'm not sure what should happen in that scenario).

On a somewhat-related note, there are a number of bugs in the initial water ui which I fixed a few months ago, but which I never ended up PR'ing as there were more bugs remaining. If you're poking around in this area then I might submit a pr today or tomorrow with what I did end up fixing.

zur003 commented 2 years ago

@hol430 - I like the FindAncestor.FindDescendant solution. That should be reasonably robust. I've submitted a PR.