APSIMInitiative / ApsimX

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

Tidy up Arbitrator and Organs #6763

Open HamishBrownPFR opened 3 years ago

HamishBrownPFR commented 3 years ago

The OrganArbitrator and associated classes as well as all the classes implementing IArbitration (organs) hold a lot of code that provides a lot of flexibility for configuring biomass supplies, demands and partitioning. However, most of the options provided are never used, there is a lot of repetition and a lot of things that should be done consistently between different plants and organs are not.
Having built and worked with others to build many different plant models in PMF I am pretty sure we can tidy up and improve our plant models if we have a much simpler (in its programming not functionality) arbitrator and a single organ type that it communicates with. Alternative arbitration approaches could be achieved by writing alternative arbitrator classes which may duplicate code but minimizes the amount of code that any specific plant model is using so makes it easier to understand, configure, document and debug biomass partitioning.

HamishBrownPFR commented 3 years ago

I have completed programming a prototype arbitrator and organ class (https://github.com/PlantandFoodResearch/ApsimX/tree/SimpleOrganArbitrator). It still needs lots of testing and debugging but I have gone far enough to know the structure will work and get some more review and opinion on the approach. Quick summary:

HamishBrownPFR commented 3 years ago

@jbrider @hol353 @hol430 @hut104. I have got this working well enough now to know it will work. There are still some bugs that need tracking down but I think it is ready for doing a code review.
We should take this opportunity to get naming conventions and the namespace and folder locations of classes right. Once this is done we will need a converter. For testing and debugging I will take a single simulation and output and graph all of the properties in all instances of OrganNutrientDelta and OrganNutrientStates. Can anyone think of a good approach to doing this. Would be cool to have a "ArbitratorTest" class that we can drop onto a simulation that outputs all the variables relating to arbitration and has a series of graphs that allows them to be reviewed easily.

jbrider commented 3 years ago

Great work @HamishBrownPFR I will have a look and see if I can get a version of sorghum working with it. (and create the meeting for Monday) A visual testing environment looks like a fantastic idea right now! I may have to do something like that to make sure sorghum outputs the same results - might not get to the visual side of it, but will be looking into the code side at least.
@hol353 @hol430 Is adding a new arbitrator table a good way to track this data? - having it be replicated during checkpoints would be handy as well?

HamishBrownPFR commented 3 years ago

Thanks @jbrider. There is at least one bug in there I still need to track down so the predictions are not spot on but it will be good to start looking at how we integrate the sorgham approaches into this.

par456 commented 1 year ago

The pull request was merged for this issue.

hol353 commented 1 year ago

Reopening this issue. I don't think this work is finished. @HamishBrownPFR and @jbrider - can you comment on where this new organ arbitrator work is going?

HamishBrownPFR commented 1 year ago

In need of time to progress this. Represents a major tidy up of the organ and arbitrator classes. Need to work out if this is a priority and how to progress if so