APSIMInitiative / ApsimX

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

Need to rethink how children of models are accessible in scripting contexts #5290

Open hol430 opened 4 years ago

hol430 commented 4 years ago

It is currently very difficult to access model children from a C# script context without using the Apsim.Get() functions. These functions are in the process of being refactored, but it shouldn't be necessary to use them at all.

For example, if a user has wheat and barley in the same paddock and wants to access wheat LAI from a manager script, what is the "correct" way of doing this? It's not easy:

[Link(ByName = true)] private IPlant wheat;

...

double lai = (double)Apsim.Get(this, "[Wheat].Leaf.LAI");
// or, in the new API:
double lai = (double)wheat.FindByPath("Leaf.LAI").Value;

This is not at all type safe, is difficult to test/maintain and is also just not very intuitive. We need to think about how we can redesign this whole system to make this easier for users.


As a starting point, perhaps the child links used by the class to interface with its children should be public properties, and components such as report should only allow access to public-facing properties (not children). In such a solution, IPlant would have a property of type ILeaf, and ILeaf would have a double property called Lai, the assumption being that all plants have leaves, and all leaves have an LAI. Then the manager script code to access LAI could be:

[Link(ByName = true)] private IPlant wheat;
// or
[Link] private Wheat wheat;

...
double lai = wheat.Leaf.Lai;

Any thoughts?

jbrider commented 4 years ago

I'm currently fighting with rotation reports (in Classic) where the crop type is not necessarily known at report time, so there are occasions where the Get is the only way to do it - particularly in scripts. Nextgen gives some better options, but working across crop types would still be problematic.

Which organs then become required/optional - it will remove the generic nature of the organs that we currently have.

I do think that our examples should use known types where possible though.

zur003 commented 4 years ago

I think @jbrider is correct in that we want to be sure that this doesn't constrain the existing generic nature of models. Is the current syntax really that bad? I admit it could use better type safety. Perhaps something like double lai = wheat.FindByPath<double>("Leaf.LAI"); would be better? (Ooops. Lost the angle brackets it my original version.)

zur003 commented 4 years ago

Or maybe that could be "ValueByPath" rather than "FindByPath".

hol353 commented 4 years ago

Guiding Principle 1: Using links is preferable to API calls (e.g. Get, Find etc). Links lead to much cleaner manager script code. API calls are a last resort option and used only rarely.

Another use case:

[Link] Plant Potato;
...
double r = Potato.Tuber.SenescenceRate;

Not currently possible for 2 reasons. The first is that the 'Plant' class doesn't have a Tuber property. The second is that 'SenescenceRate' is an IFunction, not a double. To fix the second problem we would need to change the above manager code to

double r = Potato.Tuber.SenescenceRate.Value();

which is a bit ugly.

Agreed that we don't want to lose the generic nature of model construction but I'm wondering if there is a better way that can give us some cleaner manager code.

hol430 commented 4 years ago

I don't really have a problem with .Value() although I suppose you could wrap that in a property which calls senescenceRate.Value() where senescenceRate is a private field with a link by name.

As part of this it might also be good to remove the square brackets from variable references (e.g. in report, operations, etc). I'm not sure they're really necessary - how often do we use such a path without square brackets? Personally I've never seen it, and if the square brackets are always used, then why use them at all.

hol353 commented 4 years ago

Yes, I'm suggesting we get rid of all square brackets everywhere.

HamishBrownPFR commented 4 years ago

I vote for no square brackets, they confuse me because some times they are needed and sometimes not

hol430 commented 4 years ago

@hol353 it would be nice to be able to use the code you posted above in a manager script. However I'm not sure how we can have that and also retain the generic and flexible (and yes, unsafe) model structure that we currently have. In your above example, would the property Tuber be declared in a subclass of Plant? If so, I could see that getting messy very quickly with different plant subclasses.

On a somewhat related note (and perhaps as a short-term solution), the IVariable interface and its implementations could really do with a cleanup. Many of the IVariable implementors don't implement half of the properties declared in the interface.

hol353 commented 4 years ago

How about this. We keep the Plant.cs class for model developers. It gives them all the flexibility they need. When the model is released we create a descendant class for that plant e.g. wheat. It simply includes the child models as public links. If there are similar plant models that have the same organs etc we could create a WinterCereal class that wheat derives from. Downside: It will mean quite a few small classes but we could put them in a folder under Plant. Upside: We get the ability to write double r = Potato.Tuber.SenescenceRate.Value(); in a manager script which we can't currently do.