APSIMInitiative / ApsimX

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

Can't call a method in a manager script from another manager script #4859

Open HamishBrownPFR opened 4 years ago

HamishBrownPFR commented 4 years ago

Currently, if you link to a manager model in another manager, it has not object called script so it is not possible to call methods that are declared in that manager.

hol353 commented 4 years ago

I have run up against this as well. To fix this we need to change manager to rename the users class name to something unique but usable. Before compiling it could change the word 'Script' in: public class Script : Model to the name of the Manager model prefixed to 'Script' e.g. FertiliseOnFixedDateScript. It would have to remove all spaces to make it compliant with c#. This would allow referencing from another manager model like: [Link] FertiliserOnFixedDateScript fertiliserScript; ... fertiliserScript.Amount = 10;

hol430 commented 4 years ago

This would break any scripts which reference their own class name. Instead of the manager automatically renaming the script, perhaps the user could name it something unique if they want to reference it from another script?

hol353 commented 4 years ago

Are there any examples of a manager script that references itself?

hol430 commented 4 years ago

Don't know of any in our test set, but there could well be some scripts out there which do it. It wouldn't just break manager scripts referencing themselves though, it would also prevent (or make it less intuitive to do) an Apsim.Find(this, typeof(FertiliserScript)).

hol430 commented 4 years ago

Another potential problem would be that it's possible to have multiple managers with the same name at different scoping levels. E.g. you might have two simulations, both with a manager called "fert" which are actually different in implementation. If you link to fertScript, you don't know which fertScript you will get, which is the same situation we are in currently.

rcichota commented 4 years ago

Would LinkByName (or whatever is the usage these days) provide the functionality to look for Scripts of a given Name? It would still have the last issue raised by Drew, finding different managers with the same name, but this is probably a scoping issues isn't it? Could this be resolved or minimised if the search for the Link was done bottom-up, i.e. attempts to find links at the same scope level, if unsuccessful, go one level up, and so on. (my understanding now the search is top-down...)?

hol430 commented 4 years ago

Even if you do a link by name, the problem remains that C# is a strongly-typed language and that you must specify the type of the manager script in order to access its members (unless you use reflection). If you have multiple types with the same name in the same namespace you are going to have problems. e.g.

// Inside sowing rule
public class Script
{
    ...
    // link to fert script
    [LinkByName]
    private Script FertScript;
    ...
}

I think perhaps the simplest solution would be to refactor Manager to allow the script type to be called something other than Script. That would allow the above LinkByName (and other methods of linking/finding/locating) to work. If we want to go further and - for example - implement @hol353's suggestion, then we would need to do this first anyway.

jal-frezie commented 4 years ago

Can you provide us with an example of a script in one manager including a call to a script in another? I thought this could only be done by binding the target script to an event and then having the calling script invoke that event.

hol430 commented 4 years ago

The trick is in the type definition. Instead of public class Script in each manager, they need to have different type names. E.g. one manager's type might be public class FertiliserScript, and another might be public class SowingRule. As long as they're different, that's the main thing. Then you can just link by type ([Link] private FertiliserScript otherScript;), or use this.FindInScope<FertiliserScript>() to locate it manually (replacing FertiliserScript with the type name of the other script). Here is an (extremely basic) example. Manager1 will read property X of Manager2, and write that value to the summary file.

manager-comms.zip

I've just done some experimenting with this feature, and I found a bug (imported type is defined multiple times) which seems to occur after modifying the scripts a few times. I'll look into it further and keep you updated. If you run into it, you should be able to work around it by restarting the gui.

e: updated .apsimx file so manager1 is below manager2 in the tree

hol430 commented 4 years ago

@hol353 after thinking about this some more, I'm not sure that we can really support two managers both referencing each other. Let's say we have manager script X which references manager script Y, and manager script Y also references manager script X. When we open the .apsimx file, X will fail to compile because the reference to Y cannot be resolved. Y will also fail to compile because the reference to X also cannot be resolved. I don't see a way to support this use case.

A related problem is that currently, all managers automatically reference all other managers. Given the previous issue, we may want to reconsider this behaviour. I'm thinking we could add a property to class Manager which specifies which other mangers are referenced by that manager. This could be settable from the gui (e.g. with a series of checkboxes). In the case where manager X references manager Y, the user would not be allowed to also reference X from Y. Then when a particular manager is modified, only the downstream managers (ie managers which reference the changed manager) would need to be recompiled. As opposed to the current system which leads to surprising/confusing results.

It sounds complicated, and it does add an extra step for the user when they want to setup inter-manager communications, but I think it should solve the issues with the current system where you need to close the file and reopen it each time you make a change. And it should also prevent unnecessary re-compilation of manager scripts. Let me know if it sounds ok and I'll have a go at implementing it.

jbrider commented 4 years ago

@hol430 Are you saying here that you can't use a 1 way reference - or just the 2 way? In classic I use simple events to communicate between scripts, but I couldn't get it to pass data with the event easily. Is this possible now in NextGen? If 1 way works that would still be a big improvement.

hol430 commented 4 years ago

Doing a 1-way reference is fine, and it almost works now, it's just a bit inconvenient due to the above mentioned bug where you need to re-open the file every time you change one of the scripts.

hol353 commented 4 years ago

@hol430 - give it a go and see if it works.

hol430 commented 3 years ago

One solution for 2-way communication in managers would be to compile all manager scripts into a single assembly.