APSIMInitiative / ApsimX

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

The way grids work with models needs a refactor #8760

Open hol353 opened 3 months ago

hol353 commented 3 months ago

Where is the code located?

Many models display data in grids and allow that data to be edited.

What needs to be changed?

These models implement interface IGridModel and have the property public List<GridTable> Tables. Some classes also implement the method ConvertModelToDisplay(...). Classes Physical, Forages, TreeProxy are examples of this. The interface mentioned isn't really an interface because it has implementation in it. Because of that, there a multiple ways of returning data. We need a single way of doing things. Also, the code that implements this is user interface related and not model related i.e. it isn't executed when a simulation runs. I think the models assembly should only be simulation related, and not have user interface code.

Possible implementation: The code should be moved from models into the ApsimNG assembly as a separate class for each model. These classes would be responsible for returning an ISheetDataProvider for a model:

using System.Linq;
using Models.Core;
using Models.Soils;
using UserInterface.Views;

namespace ApsimNG.SheetDataProviders
{
    public class PhysicalSheetDataProvider
    {
        /// <summary>Tabular data. Called by GUI.</summary>
        public static ISheetDataProvider GetGridTable(IModel model)
        {
            Physical physical = model as Physical;
            bool waterNodePresent = physical.FindInScope<Water>() != null;

            GridTable t = new(physical.Name);
            t.AddColumn(physical, "Depth");
            t.AddColumn(physical, "ParticleSizeSand", columnName: "Sand");
            t.AddColumn(physical, "ParticleSizeSilt", columnName: "Silt");
            t.AddColumn(physical, "ParticleSizeClay", columnName: "Clay");
            t.AddColumn(physical, "Rocks");
            t.AddColumn(physical, "BD");
            t.AddColumn(physical, "AirDry");
            t.AddColumn(physical, "LL15");
            t.AddColumn(physical, "DUL");
            t.AddColumn(physical, "SAT");

            if (waterNodePresent)
                t.AddColumn(physical, "SW", isReadOnly: !physical.IsSWSameLayerStructure);

            t.AddColumn(physical, "KS");
            t.AddColumn(physical, "BD", columnName:"Test", isCalculated: Enumerable.Repeat(true, physical.Thickness.Length));

            foreach (var soilCrop in physical.FindAllChildren<SoilCrop>())
            {
                var cropName = soilCrop.Name.Replace("Soil", "");
                t.AddColumn(soilCrop, "LL", columnName: $"{cropName} LL");
                t.AddColumn(soilCrop, "KL", columnName: $"{cropName} KL");
                t.AddColumn(soilCrop, "XF", columnName: $"{cropName} XF");
                t.AddColumn(soilCrop, "PAWCmm", columnName: $"{cropName} PAWC", columnUnits: $"{soilCrop.PAWCmm.Sum():F1} mm");
            }
            return t;
        }
    }
}

A factory could then be written to create an instance of one of these for a given model:

using System;
using System.Reflection;
using APSIM.Shared.Utilities;
using Models.Core;
using UserInterface.Views;

namespace ApsimNG.SheetDataProviders
{
    /// <summary>
    /// Provides a way to convert a IModel into a ISheetDataProvider for display in a grid.
    /// </summary>
    public class SheetDataProviderFactory
    {
        /// <summary>Get a sheet provider for a model.</summary>
        /// <param name="model">The the model to get a sheet provider for.</param>
        public static ISheetDataProvider Create(IModel model)
        {
            var dataProviderAttribute = ReflectionUtilities.GetAttribute(model.GetType(), typeof(SheetDataProviderAttribute), lookInBaseClasses: false) as SheetDataProviderAttribute;
            if (dataProviderAttribute == null)
                throw new Exception($"No SheetDataProvider attribute on class {model.Name}");

            var dataProviderType = Assembly.GetExecutingAssembly().GetType(dataProviderAttribute.ToString());
            if (dataProviderAttribute == null)
                throw new Exception($"Cannot find data provider type {dataProviderAttribute.ToString()}");

            var dataProviderMethod = dataProviderType.GetMethod("GetGridTable");
            if (dataProviderMethod == null)
                throw new Exception($"Cannot find data provider method {dataProviderAttribute.ToString()}.GetGridTable");

            return dataProviderMethod.Invoke(null, new object[] { model }) as ISheetDataProvider;
        }
    }
}

image

Why does the code need changing?

The code needs to be cleaner so that new features can be more easily added. I'm trying to add the ability to display text in cells different colours - #8757. Currently, this is more difficult than it needs to be.

par456 commented 3 months ago

Agreed.

While doing this we should also look at some of the models that are using a GridTable, but store their data in a way that makes it very difficult to map it to a GridTable. Forages for example, requires a lot of manipulation to split rows of stored date, into multiple rows for the GUI, which is very messy. It would be good practice to have the underlying structure better represent how the data is used.

I would also recommend having a generic SheetDataProvider function for the models that don't require any specialised manipulation. Something that can take a class and property and build out the required GridTable description without needed to specifically outline each thing that needs to be displayed. Otherwise we will end up with like 15 SheetDataProviders with basically the same code in them, just with the names changing.