Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

De-duplicate code associated to time-series & time-series numbers #1669

Open
1 of 2 tasks
flomnes opened this issue Oct 3, 2023 · 5 comments
Open
1 of 2 tasks

De-duplicate code associated to time-series & time-series numbers #1669

flomnes opened this issue Oct 3, 2023 · 5 comments

Comments

@flomnes
Copy link
Member

flomnes commented Oct 3, 2023

  • Binding constraints
  • Links

Main PR: #1677

Idea

One shared TimeSeriesNumbers object, used by one or many TimeSeries objects.

TimeSeries can only be constructed from an existing TimeSeriesNumbers.

#include <vector>

class TimeSeriesNumbers {
public:
  int getTSNumber(int year) const {
      tsNumbers_[year];
  }
private:
  std::vector<int> tsNumbers_;
};

class TimeSeries {
public:
  TimeSeries(const TimeSeriesNumbers& tsNumbers) : tsNumbers(tsNumbers) {}
  double getCoefficient(int year, int hourInYear) {
      return coefficients[tsNumbers.getTSNumber(year)][hourInTheYear];
  }
private:
  Matrix<> coefficients;
  const TimeSeriesNumbers& tsNumbers;
};

// Thermique pour un cluster thermique
class ThermalCluster {
public:
  ThermalCluster() : availability(tsNumbers), marketBidCost(tsNumbers) {}

  TimeSeries availability;
  TimeSeries marketBidCost;
private:
  TimeSeriesNumbers tsNumbers;
};

// Hydro pour une zone
class Hydro {
public:
  Hydro() : ror(tsNumbers), ruleCurves(tsNumbers) {}

  TimeSeries ror;
  TimeSeries ruleCurves;
private:
  TimeSeriesNumbers tsNumbers;
};

void usage() {
  ThermalCluster cluster;
  cluster.availability.getCoefficient(year, hourInYear);
  cluster.availability.getColumn(year)[hourInTheYear];
  auto& avail = cluster.availability.getColumn(year);
  a = avail[hourInYear];
  Hydro hydro;
}
@flomnes
Copy link
Member Author

flomnes commented Oct 3, 2023

Remove scratchpad.ts

@flomnes flomnes changed the title De-duplicate code associated to time-series & time-series numbers De-duplicate code associated to time-series & time-series numbers, remove scratchpad.ts Oct 3, 2023
@guilpier-code
Copy link
Contributor

guilpier-code commented Oct 3, 2023

The previous code excerpt shows a way to remove code duplication.
Each time we use an instance of Class TimeSeries, we need a vector of TS numbers as well.
So a question arises : for cohesion reason, would it make sense to gather a TS and such a vector of TS numbers in the same class ?
If it does make sense, it would allow to remove to vector of TS from business classes (ThermalCluster, Hydro, ...).
We should take into account that the same vector of TS numbers can be shared among multiple TS.
Other question : is there a design pattern for this ?

@sylvlecl
Copy link
Member

sylvlecl commented Oct 4, 2023

I suggest that:

  • the series in "business" classes are not public (ror, ruleCurves, etc): otherwise they could be re-assigned after class construction, which will break the consistency with TS numbers
  • "timeseries" is in general the word for only ONE column of data, we should probably find another name for the table (TimeseriesSet ?)
  • since in the computation, the usage will be column-wise, we should expose the timeseries (column) themselves.
    I would be in favor of having a class for it, not only a double* as today, so that the typing and naming carry the meaning of those columns, like
class Timeseries
{
public:
    // either a getter or the operator as today
    double operator[](int hour) const { return values[hour]; }
private:
    int size;
    double* values;
};

@flomnes
Copy link
Member Author

flomnes commented Oct 4, 2023

  • the usage will be column-wise, we should expose the timeseries (column) themselves.
    I would be in favor of having a class for it

Why not using TimeSeries = std::vector<double> ? Note that the size is supposed to be shared across all timeseries (rectangular matrix)

We could also use Eigen's Matrix, which supports extracting a single column through Eigen::Matrix::col

@payetvin payetvin changed the title De-duplicate code associated to time-series & time-series numbers, remove scratchpad.ts De-duplicate code associated to time-series & time-series numbers Oct 4, 2023
@sylvlecl
Copy link
Member

sylvlecl commented Oct 11, 2023

Why not using TimeSeries = std::vector<double> ? Note that the size is supposed to be shared across all timeseries (rectangular matrix)

Well it's of course a valid choice too:

  • a small difference is that with a wrapper we can add wordings and documentation to carry more meaning for our usage (typically, we can say that the operator expects an "hour" or a "timestep", not just an "index"). And possibly utilities methods if needed in the future.
  • on the other side, we don't automatically inherit of all vector operations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants