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

Remove RenouvelableParPalier from global variable #1659

Merged
merged 35 commits into from
Oct 4, 2023

Conversation

payetvin
Copy link
Contributor

@payetvin payetvin commented Sep 28, 2023

#1615

#1644 (comment)
#1644 (comment)

  • rename getAvailablePower()
  • remove scratchpad.ts
  • clean alea.cpp

@payetvin payetvin self-assigned this Sep 28, 2023
@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 28, 2023
@payetvin payetvin marked this pull request as ready for review September 29, 2023 13:36
Copy link
Member

@flomnes flomnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the precise naming for "timeStep" is hourInYear

So the caller know that it's an hour between 0 (included) and 8760 (excluded).

@@ -200,14 +200,15 @@ YString Data::RenewableCluster::getTimeSeriesModeAsString() const
return "unknown";
}

double RenewableCluster::valueAtTimeStep(uint timeSeriesIndex, uint timeStepIndex) const
double RenewableCluster::valueAtTimeStep(uint hourInYear, uint year) const
Copy link
Contributor

@guilpier-code guilpier-code Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One could expect that arguments are switched (as it was before) : first year, then hourInYear. Another way to see this, is : you cannot have an hour in the year if you don't have a year first, right ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilpier-code to be done in the next (factorization) work

@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 3, 2023
@flomnes
Copy link
Member

flomnes commented Oct 3, 2023

Rename getAvailablePower into something else

Copy link
Member

@sylvlecl sylvlecl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice:
I think we could improve again the design by returning more often column objects (double* in practice) instead of getting the year index then only retrieving the column using that index.

However, maybe that's for a future work on series classes?


uint tsFatalIndex = (uint)tsIndex.Hydraulique < ror.width ? tsIndex.Hydraulique : 0;
uint tsFatalIndex = hydroSeriesIndex < ror.width ? hydroSeriesIndex : 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the hydroSeriesIndex is used only to get this ROR index:
we could probably have a more direct getRorIndex in the series class ?

@@ -200,7 +200,7 @@ class TimeSeriesValuesHydro
// The current time-series
auto& ror = pArea->hydro.series->ror;
const unsigned int nbchro
= NumeroChroniquesTireesParPays[numSpace][pArea->index].Hydraulique;
= pArea->hydro.series->getIndex(year);
pFatalValues[numSpace] = &(ror.entry[(nbchro < ror.width ? nbchro : 0)]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be simplified too with a getRorIndex

auto& area = *(study.areas.byIndex[k]);
auto& scratchpad = area.scratchpad[numSpace];
auto& ror = area.hydro.series->ror;
auto loadSeries = area.load.series->getCoefficient(year, hourInYear);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed by @flomnes , we can retrieve the full columns only once et the start and use them (it avoids to repeat the small logic on the index)

auto const& srcinflows = inflowsmatrix[tsIndex < inflowsmatrix.width ? tsIndex : 0];
auto& mingenmatrix = area.hydro.series->mingen;
auto& mingenmatrix = hydroSeries->mingen;
auto const& srcmingen = mingenmatrix[tsIndex < mingenmatrix.width ? tsIndex : 0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have a getter for this column in the HydroSeries class, instead of going through 1. retrieve the index then 2. retrieve the column.

auto& hydroSeries = area.hydro.series;
uint tsIndex = hydroSeries->getIndex(year);

auto& inflowsmatrix = hydroSeries->storage;
auto const& srcinflows = inflowsmatrix[tsIndex < inflowsmatrix.width ? tsIndex : 0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we could have a getter for the column itself instead of the index

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@flomnes flomnes dismissed guilpier-code’s stale review October 4, 2023 11:03

The comments will be adressed in a future work

@flomnes flomnes merged commit 715d884 into develop Oct 4, 2023
@flomnes flomnes deleted the feature/remove-RenouvelableParPalier branch October 4, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants