Possible errors with scoping

[hut104]

In resolving #3554, several issues were identified. One comes from the use of a variable reference function in the inflorescence organ in White Clover that seemed to resolve the link by grabbing a function of the same name in another organ. (see #4412) There are a few ways we could think about addressing this. 1) We could stop allowing links to resolve without some sort of parent info (e.g. don't allow "[NNN]" but only something with "[PPP].NNN"). This would make things less likely, but is not foolproof. 2) Try adding [ScopedModel] to organs to stop linking across organs unless explicitly told to do so. 3) VariableReference function uses locator.get to find the function. We could get the locator to check that it only finds one matching function in scope and throw if it finds >1 - this would require developers to be explicit where scoping is inadequate to make it clear. 4) A similar issue may arise with other functions that use links. (e.g. linearinterp). Perhaps linearinterp could be changed to have XProperty as a child (which could be a VariableReference) and then the fix above is captured into that function as well. 5) We need to consider how to resolve this for expression functions as well.

[hut104]

@hol353 @hol430 @HamishBrownPFR @rcichota We need to be thinking about how we resolve this.

[HamishBrownPFR]

Explicit is always good to be sure we know what is going on. The only thing that can be safely assumed is the scope should be within the existing crop model. Hmm, actually this is not even always the case as we use soil and climate variables also.
Option 3 seems like a good bet?? If we implement something to solve this for variable reference function, we could then use variable reference functions where ever a variable is addressed. For example, instead of Linint having XProperty as a property, it would need to have a variable reference child function called XProperty. Equations in Expression function would only be able to reference children and would need to have a child variable reference function for each external reference the expression uses.

[hol430]

Another problem with the scoping rules is that any model which is not a sibling of a plant will not receive any events from the plant or any model inside the plant. E.g. if I have a management folder and I place a fertilise on sowing rule inside the management folder then the fertilise script will not receive the sowing event from the plant if the plant is not also inside the folder.

I suspect the reverse will also be true - if I have several plants in a single paddock I might decide to place them in a plant folder. Bad idea - the scoping rules will hide their events to other models which are not also in the plant folder.

[hut104]

Can we just get rid of management folders? They seem superfluous.

[HamishBrownPFR]

Superfluous form a programming perspective but A lot of people use folders for grouping their management scripts and operations.

[hol430]

Maybe folders simply shouldn't be scoped. I'm also not sure why we need a special cultivar folder.

[hut104]

I think we added folders so that we could explicitly use them to scope things (e.g. to put simulations into a folder and have all graphs just show results from simulations within the folder). They are used regularly as a grouping/scoping element.

[HamishBrownPFR]

would it be possible to get universal approach to the use of [] square brackets as part of fixing the scoping. It seems some times they are needed and sometimes not (perhaps this is report vs variable reference).

[hol430]

I think the square brackets around a model name usually indicate that the model is somewhere in scope. If you omit the square brackets then that indicates that the model is a child. That's my understanding anyway but if it's correct then there's not really any point to omitting the square brackets since any child model is guaranteed to be in scope. And if we're always going to use square brackets then there's probably not much point in forcing the use of square brackets at all - why not just always assume that the target model is somewhere in scope and forego the use of square brackets?

We really need some documentation which captures this sort of information (ScopeExample.apsimx?) so that you don't have to sift through the source code to piece together these puzzles.

[hol353]

Yes, the square brackets around a model name means go find the model in scope.

Yes, we need some documentation on this.

Yes, I would really love to remove the brackets completely. We just need to change one method in the locator (and locater) class to implicitly look in scope for a model that it can't find. Then from a paddock we could report:

• Clock.Today
• Soil.Water

etc. and the infrastructure will find clock up the top level and soil in the paddock. Looks much cleaner and easier for the user.

We also need to sit down with some use cases (with edge cases) and define what we mean by scope.

[HamishBrownPFR]

@hol353 @hut104 @hol430. Is there any reason why the error trap highlighted below is commented out? in #4538 wheat failing the performance tests and I have isolated it to a couple of variable references where the locator is returning nulls.
In this pull request I have changed linint so it doesn't locate the x value itself, instead it has a child property called Xvalue and I have written a converter that puts the xproperty into a variableReference child function looking for the xproperty reference. Problem is there are a couple of references where linint located values but variable reference does not. With the current variable refference code If it drops into the final else and o is null it will return a zero. I can fix this and put in an error test but was wondering why it is commented out in the first place

[hol430]

That if (o == null) test is commented out because we can't get jenkins to run green if it's enabled - we're working on it.

Problem is there are a couple of references where linint located values but variable reference does not.

Interesting - which particular file/linear interp are you referring to here? That sounds like a bug.

[hut104]

The build fails big time if we uncomment that test. The system cannot even handle references to row spacing in the sowing data because sowing data is null prior to sowing. I un-comment the line to incrementally work through the build, fixing one issue at a time. Some broken links can take a day or two to resolve if fixing the reference significantly changes the model behaviour. We desperately need to fix the remaining broken links.

[HamishBrownPFR]

The two highlighted here. They look for a variable that is a sibling. Not sure why they don't work

[hut104]

Try removing .Value()

[hol430]

Maybe it needs square brackets around VPD?

[HamishBrownPFR]

@hol353, @hol430, @hut104 Some progress has been made here with the merging of #4538. We now have linint getting its xvalue from a child function and have an converter that puts a variable reference function on existing linints. Redundant .Value() in references have also been removed.
I haven't been able to work out how to get ExperssionFunction using variablereference for its variable gets. So how do we keep progressing this now? I noticed that Allometric Demand function is also doing gets of string properties. I change this to use variable references instead and do a bit of a scan to see if there are any other places in the code. Someone who is better at programming that me would need to tackle the ExperssionFunction. Then are we in a position to implement some more robust scoping rules??

[hol353]

There are many instances of ExpressionFunctions referencing variables in other models or other parts of the same model e.g. nutrient. We can't restrict ExpressionFunctions to just child functions. Also, it seems a bit cumbersome to have a child variable reference for every variable that an ExpressionFunction uses. Some of the ones in Nutrient use 5 or 6 in the same expression.

[HamishBrownPFR]

We could restrict expression functions to using child functions and the child functions could be variable references that bring the values from else where. However I am not sure what we would gain by doing this.

[HamishBrownPFR]

Other than reducing the number of classes that may be applying slightly differnt uses of Locator making the tidying up of scoping harder?

[HamishBrownPFR]

Sorry, I am trying to be helpfull here as I don't like the idea that or models are not doing what we thing they are doing. However, I am not sure I am fully comprehending what we need to do to fix the scoping. Seems that each time there is a locator method used in the code it can return different results in some situations. This happened when I replaced the locator code in Linint with a variableReference function that was also using the locator but returning different values for a couple of wheat functions. We also have APSIM.get() being use to locate values from text string addresses and this is used alot more than locator. Are both locator and APSIM.get() needed, can these be merged. Do we also need to minimise the amount of code that is using APSIM.get() to reduce issues with subtle variations in application giving dufferent results like we saw with locator above?

[HamishBrownPFR]

Perhaps I would be best to focus the we bits of time I have on fixing incorrect paths in VariableReference instances so we can turn the null error check back on?

[hol353]

Unsure why the locator gives different results based on where it is called from. Would need to do some debugging.

The retrieval code needs a complete rewrite. There are way too many edge cases that have been added on over the years and they don't have unit tests.

I'm also interested in trying to define what the scoping should be e.g.

• Should everything inside a zone be in scope? Much simpler and more intuitive?
• PMF plant sends out events (e.g. 'PlantSowing') that child organs receive. These shouldn't go to organs of other plants in the zone. Perhaps we shouldn't use events for this.
• I suspect folders shouldn't define scope. They do currently.

There are probably other use cases as well.

[HamishBrownPFR]

It is kind of academic because Linint no longer calls locator. However I suspect the difference was in the way the arguments were entered rather than where locator was called from. Linint did it like this: string PropertyName = XProperty; object v = locator.Get(PropertyName);

Variable reference does it like this: public string VariableName { get; set; } object o = locator.Get(VariableName.Trim());

In the two examples where they gave different answers, the file path was "VPD.Value()". The way it was implemented in Linint was able to find the correct value but in Variable reference it failed to find a variable until square brackets were put around the VPD.

These sorts of subtlities will make refactoring the scoping a challenge so it would be worth either implementing a consistent approach to calling them and/or minimising the number of places they are used in the code to ease the process? To that end, do we need locator or can it be replaced with APSIM.get() ??

[hol353]

I'm not sure creating VariableReference nodes everywhere is the right thing. Yet more complexity for the user to deal with - they need to remember to put a child node onto linint and other functions.

Your code examples above (linint vs variable reference) looks identical and should give the same result. Something else is gong on there. Also, Apsim.Get calls locator.Get so it should be equivalent as well.

Locator.Get is the function that needs refactoring and extensive unit testing. Eventually, I'd like to get rid of the Apsim.Get and replace calls to it with Locator.Get. One, single, well tested class (Locator) for getting and finding models and variables is what we need.

[hut104]

Apsim.Get is called in manager scripts. I wonder if Apsim.get is the method to retain because for Joe Average that code makes sense (ie do a get from APSIM to get some data).

[hol353]

The alternative: [Link] Locator locator; ... locator.Get('wheat.yield');

One more line of code but one less argument. No need for a 'this' argument. Here's the Apsim.Get line of code for comparison: Apsim.Get(this, 'wheat.yield');

[hol430]

How do you get away with removing the 'this' argument? The models in scope will vary depending on the model which we pass as an argument into Apsim.Get.

[peter-devoil]

I'm hoping that we can end up with an api here that can be used by several components that do much the same thing (report UI, intellisense etc) in a consistent manner.

[hol353]

When the link is resolved by the infrastructure, an instance of the locator is created with the model instance as the argument to the constructor: internal Locator(IModel relativeTo)

This means the 'this' argument doesn't need to be specified on the Get call. Simpler for user?

[hol353]

Responding to @peter-devoil Yes I agree we need a single api that everything uses.

[HamishBrownPFR]

How do we come up with a concrete plan to progress things here??

[hol353]

Sadly nothing is probably going to happen. I would love to spend several weeks designing and implementing something but that isn't going to happen in the next year. There probably isn't anyone else that has the time to do it either.

[hol430]

When the link is resolved by the infrastructure, an instance of the locator is created with the model instance as the argument to the constructor: internal Locator(IModel relativeTo)

This means the 'this' argument doesn't need to be specified on the Get call. Simpler for user?

Simpler for the user perhaps, but if this is going to replace Apsim.Get(...) then we lose the ability to choose which model defines the scope. Probably not useful very often but sometimes it does come in handy.

[hol353]

There is nothing stopping the developer from creating their own instance of Locator, passing in the 'relativeTo' argument to the constructor.

[hol430]

True - I guess we could just make the constructor public.

[hol353]

Time to revisit this issue. How about this for a plan?

1. Let's change scoping to use Zone's only. Anything in a zone or it's parent zone is in scope.
2. Remove the concept of a [ScopedModel].
3. Change the way graphs find data. Change them to progressively look for data in a zone, simulation, folder, experiment, whole file until they find data to graph. i.e. graphs don't use the above described scoping mechanism. Perhaps call this graph scoping instead of model scoping.
4. All links to IFunction instances should be LinkType.Child. Perhaps this can be enforced. This gets around the problem of an organ linking to the wrong function under another organ.
5. We remove the concept of square brackets to mean go find this object in scope. Instead the get API call can be modified slightly to find an object for the first part of a given path. e.g. Apsim.Get(this, 'Wheat.Leaf') instructs the locator to find a wheat model in scope and then a Leaf property of that model. Shouldn't be a big job. Much bigger job writing a converter to strip square brackets from everywhere in a .apsimx file and getting Jenkins green.
6. We update the documentation to describe the new approaches listed here.
7. @hol430 Implements the above dot points.

This is much simpler. This is more intuitive. Easier to document.

Will need to be careful when implementing dot point 1 because plant events will now go everywhere in a zone. If you have two plants in the zone then the organs of one plant will get the events from the other plant, not their parent. This can be overcome by the organs checking where their 'sowing' event is coming from. Alternatively perhaps all organs should have methods that get called by the parent plant when sowing etc happens i.e. don't use events for communicating between a parent plant and it's child organs.

[hut104]

What could possibly go wrong.....?

---

From: Dean Holzworth notifications@github.com Sent: Friday, 12 June 2020 1:37 PM To: APSIMInitiative/ApsimX ApsimX@noreply.github.com Cc: Huth, Neil (A&F, Toowoomba) Neil.Huth@csiro.au; Mention mention@noreply.github.com Subject: Re: [APSIMInitiative/ApsimX] Possible errors with scoping (#4423)

Time to revisit this issue. How about this for a plan?

1. Let's change scoping to use Zone's only. Anything in a zone or it's parent zone is in scope.
2. Remove the concept of a [ScopedModel].
3. Change the way graphs find data. Change them to progressively look for data in a zone, simulation, folder, experiment, whole file until they find data to graph. i.e. graphs don't use the above described scoping mechanism. Perhaps call this graph scoping instead of model scoping.
4. All links to IFunction instances should be LinkType.Child. Perhaps this can be enforced. This gets around the problem of an organ linking to the wrong function under another organ.
5. We remove the concept of square brackets to mean go find this object in scope. Instead the get API call can be modified slightly to find an object for the first part of a given path. e.g. Apsim.Get(this, 'Wheat.Leaf') instructs the locator to find a wheat model in scope and then a Leaf property of that model. Shouldn't be a big job. Much bigger job writing a converter to strip square brackets from everywhere in a .apsimx file and getting Jenkins green.
6. @hol430https://github.com/hol430 Implements the above dot points.

This is much simpler. This is more intuitive. Easier to document.

Will need to be careful when implementing dot point 1 because plant events will now go everywhere in a zone. If you have two plants in the zone then the organs of one plant will get the events from the other plant, not their parent. This can be overcome by the organs checking where their 'sowing' event is coming from. Alternatively perhaps all organs should have methods that get called by the parent plant when sowing etc happens i.e. don't use events for communicating between a parent plant and it's child organs.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/APSIMInitiative/ApsimX/issues/4423#issuecomment-643046953, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC2UVWUAWNTC5UZILFNCJXTRWGPIZANCNFSM4JSAHE6Q.

[hol430]

So under the implementation you described, you could put a variable reference under wheat which points to the variable "[Tuber]" (tuba?) and it finds the tuber inside potato?

Or a more realistic example, you have wheat and potato in the same paddock, but you remove the juvenile phase from wheat, and now any variable reference which points to the juvenile phase is now finding potato's juvenile phase.

Edit: wheat != leaf

[HamishBrownPFR]

A plant is encapsulated. With the exception of things in variable reference and expression functions everything is going to be within the scope of the Parent plant. Can we have scoping in plant constrained to the parent plant. I.e when scoping, look for plant in ancestry and limit scoping to that. The variable reference function already uses APSIM.get() internally so would be alright. Linint no longer searches its xproperty, it uses a variable reference. Expression functions might still be a problem. The same could be true for other models, have a list of parent models (IPlant, ILifeCycle etc) where scope will always be within that model and have scoping stop going up at that point.
Would be great to get rid of square brackets

[hut104]

Should we have more checks? e.g. if you find >1 object throw error

---

From: Hamish Brown notifications@github.com Sent: Saturday, 13 June 2020 4:32 AM To: APSIMInitiative/ApsimX ApsimX@noreply.github.com Cc: Huth, Neil (A&F, Toowoomba) Neil.Huth@csiro.au; Mention mention@noreply.github.com Subject: Re: [APSIMInitiative/ApsimX] Possible errors with scoping (#4423)

A plant is encapsulated. With the exception of things in variable reference and expression functions everything is going to be within the scope of the Parent plant. Can we have scoping in plant constrained to the parent plant. I.e when scoping, look for plant in ancestry and limit scoping to that. The variable reference function already uses APSIM.get() internally so would be alright. Linint no longer searches its xproperty, it uses a variable reference. Expression functions might still be a problem. The same could be true for other models, have a list of parent models (IPlant, ILifeCycle etc) where scope will always be within that model and have scoping stop going up at that point. Would be great to get rid of square brackets

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/APSIMInitiative/ApsimX/issues/4423#issuecomment-643424983, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC2UVWWBOV7KDLSUPBO4EPLRWJYCVANCNFSM4JSAHE6Q.

[hol353]

re @hut104: I think we already throw an exception if it finds > 1 match.

re @HamishBrownPFR: Yes, we could have scoping constrained by the 'parent' model. That's what we do now. But in reality every model/function has a parent all the way up to the top level Simulations model. There is no difference between a low level function and a model like say Clock. That's why we introduced the concept of a scoped model to denote that some models (e.g. plant) have special meaning when scoping is considered.

re @hol430 : Yep, a VariableReference reference under one plant will find a match under another crop. Likewise it will find a match under another organ which is usually equally bad and is a problem we have now. What about this for a scoping rule: All children are in scope, all siblings (but not their children) are in scope, repeat like this recursing up through all parents to the top level of the simulation. If you need more flexibility to get at things not in scope then a path will have to be used. Does this work better?

[hol430]

Well manager folders would still cause problems. E.g. a manager in a manager folder would not be in scope of a plant that is a child of the zone.

[hol353]

But wouldn't the manager folder be a sibling of the plant? This makes it in scope.

[hut104]

If it gets in the way, we get rid of folders in simulations.

---

From: Dean Holzworth notifications@github.com Sent: Tuesday, 16 June 2020 1:30 PM To: APSIMInitiative/ApsimX ApsimX@noreply.github.com Cc: Huth, Neil (A&F, Toowoomba) Neil.Huth@csiro.au; Mention mention@noreply.github.com Subject: Re: [APSIMInitiative/ApsimX] Possible errors with scoping (#4423)

But wouldn't the manager folder be a sibling of the plant? This makes it in scope.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/APSIMInitiative/ApsimX/issues/4423#issuecomment-644510125, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC2UVWT64B5GBKBBRMTIUCLRW3RODANCNFSM4JSAHE6Q.

[hol430]

Yep, the manager folder would be in scope of the plant. However the manager inside the folder would not be in scope.

[hol430]

5149 was a duplicate - see there for discussion/diagram.