Scenario builder 2 : internal design is messy

[guilpier-code]

This page discusses a particular aspect of this more general discussion

The yearly variation classes are the classes that contain the scenario builder 2d-matrices, and methods to act on it.
The class Rules (class for a rule set), naturally contains an instance of each yearly variation class
Depending on the scenario category they are associated to , the yearly variation classes are currently named LoadTSNumberData, HydroTSNumberData, BindingConstraintsTSNumberData, ...

Yearly variation class hierarchy is messy :

• inheritance dependencies does not seem to make sense, and is too complicated. There are several level of inheritance and some re-definitions of concrete methods. In this current case, inheritance is questionnable.
• the classes seem to have too many and unclear responsibilities. For instance these classes have the responsibility to save their content into the scenariobuilder.dat file, but curiously not to read it. My personal choice would be to remove the saving from the classes, and at least do like it's done for reading.
• code duplications over classes : for instance in methods saveToINIFile and apply.
• wrong names :
  • saveToINIFile saves into a .dat file : strange isn't it (But maybe we cannot do much about it) ?
  • apply(...) step : name expressive enough ?
• In class Rules (the container of several scenario categories) : in a daily chat, the following requirement came up : it would be great if, instead of having an instance of each yearly variation class, we had a std container to hold them all. It means that we should have a common base (and probably abstract class) for any yearly variation class.

Let's recall the requirements :

• No special requirement on class Sets
• We already suggested some changes on class Rules's interface in this other discussion, so we should take them into account
• A common [abstract] base class for any yearly variation class, despite the differences between scenario categories
• Design for change : what if we add a new scenario category ?
• No duplication code : which indicator can help us notice code duplication was reduced ? In one hand, the number of classes can increase (if responsibilities are split), and extra code will appear (declarations, methods prototype, ...). On the other hand, removing code duplication makes the code shrink.
  By the way, favoring composition over inheritance and injection should help us split the responsibilities.

[guilpier-code]

How scenario builder could be designed internally

For yearly variation classes, I seriously think about the composite design pattern : the tree in the composite design pattern would be about the locations. Indeed, some scenario categories have a single location, some other have a second location, inside the first one.

Notice that we have an embarassing dependency to the study in the code shown below. The next step is to try to remove this dependency. For this, please see this discussion.

The main idea :

Rules::Rules(Study& study, std::string name) : name_(name)
{
    // ---------------------------
    // Wind production scenario
    // ---------------------------
    // ... We need to extract and make data from study for wind. See below for more details.
    auto windMatrixProperties = std::make_unique<WindMatrixProperties>(study);
    windMatrixProperties->build();

    // ... We create a new scenario category (for wind)
    auto windScenario = std::make_unique<SingleScenarioCategory>(windMatrixProperties);

    // ... We add it to the list of scenario category
    categories_["Wind"] = windScenario;
    ... 

    // -------------------------------------
    // Thermal production scenario
    // ------------------------------------
    // ... First create a MULTIPLE scenario category
    auto thermalScenario = std::make_unique<MultipleScenarioCategory>( /* no arg needed */ );

    for(auto& area : study.areas)
    {
        auto thermalMatrixProperties = std::make_unique<ThermalMatrixProperties>(area);
        thermalMatrixProperties->build();
        auto areaThermalScenario = std::make_unique<SingleScenarioCategory>(area->name, thermalMatrixProperties);
        thermalScenario->add(area->name, areaThermalScenario);
    }
    categories_["Thermal"] = thermalScenario;
    ...
}

// On the client side
int main(...)
{
    ... 
    ScenarioBuilder::Rules rules("Custom rules", study);
    
    // Set a scenario builder value
    rules.category("Thermal").location("Area 1").location("some cluster").year(2).setValue(5);
    rules.category("Wind").location("Area 2").year(0).setValue(1);
   
   // Save scenario builder rule set
   rules.saveToINIFile(file); // See below the Rules::saveToINIFile(...)
}

---

More details (feasibility ?)

Among all scenario categories, what are the things that are different ? And what are things that are the same ?
Today scenario categories classes hierarchy struggle to make these differences. That's the reason why there are messy.
If we identify the differences between all scenario, we can isolate them in classes, and inject them in a generic class meant to do what all scenario category do.

Particular category properties classes

This classes are the only one that are particular to each scenario category (thermal, wind, hydro level, link capacities, ...). They are mainly used to fetch and store data about a particular scenario category from the study (reference on target matrices, value checker,... ).
The classes that actually act on scenarii (set/get a value, save to .ini file, resize matrices, ...) are generic classes (see next section). This avoiding code duplication.
In order to build a "leaf" (in the sense of the composite design pattern) scenario category, we need to extract and make data from study :

• We need to fetch (in all locations [area or cluster]) a reference to the TS number 1d-matrices (= columns) that will be modified when executing the apply step. Typically, for wind, it would be area.wind.series.timeseriesNumbers.
• We also need a value checker, in order to check if a value set by user in a scenario category 2d-matrix is correct.
  • For thermal scenario, the locations are clusters. The value associated to a cluster and a year is a TS number, and a requirement on this number is that it must be smaller than the number of TS that the available production of a cluster has. So a value checker is associated to each cluster to check that.
  • For wind scenario, the locations are areas. The value associated to an area and a year is a TS number as well. For Wind the same value checker is associated to each area, because we know all areas have the same number of wind TS.
• Locations names : we need the names of the locations the columns matrices are associated to. For wind, it would be area names. For thermal, it would be cluster names.

// Struct aggregating some properties of a column of a 2d-matrix.
// Typically, for a thermal cluster : a name, a value checker, and a target column to be modified at apply step.
struct ColumnProperties
{
    Matrix<Yuni::uint32>& targetColumn; // 1d-matrix or Matrix::ColumnType
    ValueChecker valueChecker;
    std::string locationName; // A area name, a cluster name, ...
}

// Template method pattern
class MatrixProperties
{
public:
    void build();
    virtual std::string id() = 0; // The old prefix is renamed into id ("t" for thermal, "w" for wind, ... )

private: // Member functions
    virtual void collectTargetColumns() = 0;
    virtual void collectLocationNames() = 0;
    virtual void defineValueCheckers() = 0;
private:
    std::vector<ColumnProperties> columnsProperties_; // Size : if, wind : as much as areas. If thermal, as much as clusters.
}

void MatrixProperties::build()
{
    collectTargetColumns();
    collectLocationNames();
    defineValueCheckers();
}

class WindMatrixProperties : public MatrixProperties
{
public:
    WindMatrixProperties(Study& study) : study_(study) {}
    
    std::string id() override { return "w"; }
    void collectTargetColumns() override;
    void collectLocationNames() override;
    void defineValueCheckers() override;
    
private:
    Study& study_;
}

class ThermalMatrixProperties : public MatrixProperties
{
public:
    ThermalMatrixProperties (Area& area) : area_(area) {}
    
    std::string id() override { return "t"; }
    void collectTargetColumns() override;
    void collectLocationNames() override;
    void defineValueCheckers() override;
private:
    Area& area_;
}

Generic classes for the scenario categories

We have only 2 classes for scenario categories, whether there are associated to Thermal productions, wind production, ....
The particularities of each category are represented by matrix properties classes (see above).
Either the scenario is a single location scenario category (wind, hydro level, load, ...) or a multiple locations category (thermal, renewable).
What about link capacities ?? Single or multiple ? Remains to be found out.

// Base class for a scenario category
class IScenarioCategory
{
public:
    virtual void reset() = 0;
    virtual IScenarioCategory& location(std::string location) = 0;
    virtual void setValue(double value) = 0;
    virtual void saveToINIFile(Yuni::IO::File::Stream& file) const = 0;

   IScenarioCategory& year(unsigned int y);
    
protected:
    unsigned int storedYear_ = 0;  // Used after call of year(unsigned int y)
}

IScenarioCategory& IScenarioCategory::year(unsigned int y)
{
    currentYear_ = y;
    return *this;
}

// Class for a single location category
class SingleScenarioCategory : public IScenarioCategory
{
public:
    SingleScenarioCategory(std::string location = std::string(), std::unique_ptr<MatrixProperties> matrixProperties);
 
    IScenarioCategory& location(std::string location) override;
    void saveToINIFile(Yuni::IO::File::Stream& file) const override;
    void setValue(double value) override;
   
private:

    // Data members
    std::unique_ptr<MatrixProperties> matrixProperties_;
    Matrix<Yuni::uint32> matrix_; // Scenario 2d-matrix
    std::string storedLocation_; // Used to store a location after call of location(std::string)
    std::string lineStart_;
    
    // The single category location : empty by default, but filled with an area name if category has 2 location coordinates 
    std::string location_;
}

SingleScenarioCategory::SingleScenarioCategory(std::string location, std::unique_ptr<MatrixProperties> matrixProperties)
        : location_(location), matrixProperties_(matrixProperties)
{
    lineStart_ = matrixProperties_->id() + ", ";
    if (!location_.empty())
        lineStart_ = location_ + ", ";
}

IScenarioCategory& SingleScenarioCategory::location(std::string location) 
{ 
    storedLocation_= location;
    return *this; 
};

void SingleScenarioCategory::setValue(double value)
{ 
    unsigned int locationIndex = matrixProperties_->locationIndex(storedLocation_);
    matrix_[locationIndex][storedYear_ ] = value;
}

void SingleScenarioCategory::saveToINIFile(Yuni::IO::File::Stream& file) const
{
    for(unsigned int locationIndex = 0; locationIndex  < matrix_.width; locationIndex++)
    {
        std::string locationName = matrixProperties_->locationNames(locationIndex) + ", ";
        for(unsigned int year = 0; year < matrix_.height; year++)
        {
            file << lineStart_ << locationName << year << " = " << matrix_[locationIndex][year];
        }
    }
}

// Class for multiple location scenario : for example the thermal clusters production. 
// There are several clusters in an area. 
class MultipleScenarioCategory : public IScenarioCategory
{
public:
    MultipleScenarioCategory() = default;
    IScenarioCategory& location(std::string location) override;
    void setValue(double value) override { /* Does nothing */ }
    void saveToINIFile(Yuni::IO::File::Stream& file) override const;

    void add(std::unique_ptr<IScenarioCategory> component);

private:
    // Example : associate an area name to a  SingleScenarioCategory  (a 2d-matrix)
    std::map<std::string, std::unique_ptr<IScenarioCategory>> components_;
}

IScenarioCategory& MultipleScenarioCategory::location(std::string location) 
{ 
    return *(components_[location]);
};
void  MultipleScenarioCategory::saveToINIFile(Yuni::IO::File::Stream& file) const
{
    for(auto& component : components_)
        component.saveToINIFile(file);
}

void MultipleScenarioCategory::add(std::string areaName, std::unique_ptr<IScenarioCategory> component)
{
    components_[areaName] = component;
}

class Rules
{
public:
    Rules(Study& study, std::string name);
    ~Rules() = default;
    
    IScenarioCategory* category(std::string name);
    void saveToINIFile(Yuni::IO::File::Stream& file) const;
    
    bool reset();
    bool apply();
    ...

private: // Data members
    std::string name_; // name of the current rule set

    // Associates a name ("Thermal", "NTC", "Load", ...) to a category object
    std::map<std::string, std::unique_ptr<IScenarioCategory>> categories_;  // smart ptr mandatory for polymorphism
}

Rules::Rules(Study& study, std::string name)
{
    // See above
}

IScenarioCategory* Rules::category(std::string name)
{
    return categories_[name];
]

void Rules::saveToINIFile(Yuni::IO::File::Stream& file) const
{
    for (auto& category : categories_)
        category->second.saveToINIFile(file);
}

Requirements fulfilled ?

• Class Rules's interface in this other discussion : follows the requirements of the new Rules's interface
• Code duplication : we have generic classes that contain data and actions common to all scenario classes. Particular data are contained in classes particular to scenario categories.
• Design for change (add a new scenario category) : in order to add a new scenario category, we only need to define a new MatrixProperties class. All actions on any scenario category is already taken care of by generic classes

[sylvlecl]

I may not have all the details in mind yet, but I think we could simplify a lot more the design, and make some things more explicit in the data model, in particular the indexing on years/areas/clusters.
And also, probably use even less inheritance in favor of plain data classes and functions. I don't think there is a lot of added values for abstractions here.

• About the data classes:
  First, I think we should stop using "matrices" here, and use a more object oriented model to be able to name things. The matrix representation only make it more opaque (what are the dimensions of those matrices ? what data do they represent ? actually sometimes it's 1-dimension matrices, which is not clear in the class)
• About serialization
  We can start to apply our ADR about single responsibilities by extracting the serialization functions from the data classes
• About processing:
  Again to separate responsibilities, we should make the separation between the data classes and the processing very clear.
  The use of the "rules" (which is a bad name I think, there is no rule, only a simple mapping) to modify the study should be a function of its own, outside of the data classes.
  Actually, this function should probably go the solver lib, because it's really a processing function

For me the whole "public API" of this module could be something as simple as:

// First, definition of the data classes, with no "behaviour"

// For one type of data (load...), mapping of year/location to TS number
template <class Loc>
class TimeseriesNumbers
{
public:
    //public access methods, only what's necessary/useful
private:
    std::map<Loc, uint32> timeseriesNumbers;
};

// plain structs for index of the map,
// with explicit names for the indices
struct AreaLocation
{
    uint32 areaIndex;
    uint32 year;
};

struct ClusterLocation
{
    uint32 areaIndex;
    uint32 clusterIndex;
    uint32 year;
};

class Rules  //to be renamed?
{
public:
private:
    TimeseriesNumbers<AreaLocation> loadTimeseriesNumbers;
    TimeseriesNumbers<ClusterLocation> thermalTimeseriesNumbers;
    // ... other types here ...
};

// Serialization free functions
Rules loadRulesFromIniFile(const std::string& filename);
void saveRulesToIniFile(const Rules& rules, const std::string& filename);

// Processing function: copy those rules into the study (the "apply" of today)
// Should probably go in solver lib instead
void copyRulesToStudy(const Rules& rules, Study& study);

[guilpier-code]

About the data classes:
First, I think we should stop using "matrices" here, and use a more object oriented model to be able to name things. The matrix representation only make it more opaque (what are the dimensions of those matrices ? what data do they represent ? actually sometimes it's 1-dimension matrices, which is not clear in the class).

It's interesting because of the simplicity of the map to reach any "cell" of any scenario category, we should have a chat about it.
But how to implement this internally ? It certainly will be quite complex. And how would you add easily a new scenario ?
It's a complete change compared to how it's made currently. It would though require some work : because the way we store TS numbers or levels changes (no more matrices). It would have consequences elsewhere, for instance in the TS number random generation (see timeseries-numbers.cpp).

About serialization
We can start to apply our ADR about single responsibilities by extracting the serialization functions from the data classes

I do agree.

About processing:
Again to separate responsibilities, we should make the separation between the data classes and the processing very clear.
The use of the "rules" (which is a bad name I think, there is no rule, only a simple mapping) to modify the study should be a > function of its own, outside of the data classes.
Actually, this function should probably go the solver lib, because it's really a processing function

Wow, it's radical ! So we always should separate data from processing in this way for any class ?
Methods can have benefits.

About your piece of code :
class Rules owns a data for each category. Among requirements for a re-design, we have to make sure that all the scenario categories are stored inside a list of common type objects.