APSIMInitiative / ApsimX

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

Plant events are not seen by Manager #5149

Closed rcichota closed 3 years ago

rcichota commented 4 years ago

I dropped the "Fertilise at sowing" Manager that is on the Apsim ToolBox to a simulation and could make it work. It should be responding to the OnSowing event, but it not. The plant is been sown and the event can be used (e.g. report). I then tried OnPlantSowing and OnPlantEnding and no response either. There are simulation that use the same manager and they work, it turned out the difference is that I always use a Folder to hold my managers and this somehow isolates the scripts from some events... The immediate solution was to keep that script out of the folder, but that is quite annoying, having all the manager in one place is easy to handle specially if having a few of them... Also there was no way to figure out what was the problem, it took me a few hours and having to reconstruct the simulation base on the one working to see this issue. I am sure i am not the only one using folders to organise the simulations.

rcichota commented 4 years ago

I noticed this is actually an scope issue, already in a comment in #4423 I am not sure about the wider implications, or what it would take to fix this, but folders have been used to group things in Apsim for a long time and are quite handy. It would be great to have them working. A major problem with the current situation is that one has to know where they will work or not, really hard to debug as it is not an error...

BrianCollinss commented 4 years ago

Very long time ago I opened an issue reporting that Managers in Folders don't see sowing and harvest events. It was supposed to be fixed by a change in scoping. No idea what happened since then.

hol430 commented 4 years ago

Yes, this is the same as #3383, which was never resolved. The problem here is that a folder is a scoped model, which causes its children to not receive events from any siblings which are also scoped models (Plant is a scoped model).

Folders need to be scoped models in order for graph scoping to work - a good example is in the potato test set. Under the validation folder is a folder called Australia. In the Australia folder are some experiments and some graphs which show data from the Australian experiments. If folder is not a scoped model, these graphs will show data from the New Zealand folder as well.

I don't know what the best solution would be here. Maybe we need to have a serious rethink about how scoping works. There was more discussion about this in #4423 where we also couldn't come to a decision.

hol430 commented 4 years ago

@hol353 what do you think of this:

image

hol353 commented 4 years ago

Looks good to me. Well done.

hol353 commented 4 years ago

Oh another change needed to the image: rename the top level manager script to something else e.g. script2 and write a couple of use cases for what is in scope for it.

hol430 commented 4 years ago

What about the scenario of nested zones. The scoping rules in the image above are perhaps a little ambiguous on this point. My interpretation would be "if x and y share a Zone ancestor, and y is visible, then y is in scope of x",

E.g. in the below image HarvestReport would be in scope of wheat from field2 and field3, but not field 4. Is that the correct behaviour?

image

hol353 commented 4 years ago

Correct (I think). And I think this is ok. I don't know of any cases where we have nested fields.

HamishBrownPFR commented 4 years ago

Correct (I think). And I think this is ok. I don't know of any cases where we have nested fields.

I don't think nested zones should even be allowed. Zones abstract an area, if we want to split it into smaller areas we add another zone at the simulation level. Putting one zone inside another does not really make sense conceptually to me

HamishBrownPFR commented 4 years ago

@hol430 what about multi zone simulations where the plant has root organs in both zones. See C:\GitHubRepos\ApsimX\Prototypes\Orchard\Orchard.apsimx for example.

HamishBrownPFR commented 4 years ago

I don't think c is correct. There are plenty of cases where varref would need to find x. Are the scoping rules relevent to Var Ref because it uses .get() ? image

hol353 commented 4 years ago

When a path is specified that has a model name inside of square brackets, scoping rules are used to find the model inside the square brackets. So Phenology will be found ok by scoping and then ThermalTime will be found as a child.

hol353 commented 4 years ago

r.e. the multi root zone simulations: This will work ok as well. The SetupTreeRootZones manager script gives the root objects the name of the zone where the roots are. The root object then gets that zone and then uses scoping (relative to that zone) to get the water balance in that zone. It will work.

Well done for trying to think up some use cases that might break the above changes to scoping. The changes we're suggesting above only really affect scoping within a zone not beyond.

HamishBrownPFR commented 4 years ago

When a path is specified that has a model name inside of square brackets, scoping rules are used to find the model inside the square brackets. So Phenology will be found ok by scoping and then ThermalTime will be found as a child.

Wern't we wanting to get rid of the square brackets?

hol430 commented 4 years ago

When we get rid of square brackets, the term before the first dot will still denote "some model in scope". So in this case, as long as Phenology is in scope, it will be able to find ThermalTime as a child of Phenology.

hol430 commented 4 years ago

I've updated the diagram as per suggestions. What is the consensus on nested zones? Currently it's possible as long as the child zones' total area does not exceed that of the parent. It makes sense to me that you can divide a zone up into smaller sub-zones, although I'm not sure why you would want to.

scope-rules

hol353 commented 4 years ago

Having nested zones is probably useful. I think the above scoping rules allow for that.

The diagram should name the 2 manager scripts so that the use cases a) through f) are not ambiguous.

rcichota commented 4 years ago

This is looking good indeed. I don't of any use now, but giving the increased need to do simulations that are spatially-aware (to some extent), being able to use nested zones sound like could be useful.

her123 commented 4 years ago

Just clarifying my understanding of the diagram @hol430 . Clock is a child of Zone? Manager Folder is a child of clock? Is this a hierarchical diagram?

hol430 commented 4 years ago

Good point, it is a bit inconsistent. Models connected by horizontal lines are supposed to be siblings. Models connected by vertical lines are supposed to be parent/child. So clock is a sibling of zone, and manager folder is also a sibling of zone (and clock). However this is not consistent with how wheat/potato are shown.

her123 commented 4 years ago

Would it be good to have an official hierarchical diagram that can be used to explain the rules to the modeller?

hol430 commented 4 years ago

Yep, the idea is that this diagram will eventually make its way into the docs somewhere. I have updated it to make the parent/child hierarchy a bit more clear.

scope-rules

hol430 commented 3 years ago

Closing as duplicate of #4423