Skip to content

Feature/multiple rule curve cr 24#113

Open
nikolaredstork wants to merge 87 commits intodevelopfrom
feature/multiple-rule-curve-cr-24
Open

Feature/multiple rule curve cr 24#113
nikolaredstork wants to merge 87 commits intodevelopfrom
feature/multiple-rule-curve-cr-24

Conversation

@nikolaredstork
Copy link

@nikolaredstork nikolaredstork commented Dec 9, 2024

This PR implements the Multiple Rule Curves for Hydro Reservoirs as an optional new feature, RTE-I's CR24.
Specification: https://rteinternational.sharepoint.com/:w:/s/AntaresUser'sClub-Interne/EbkQpgbTrtBIkT2jQiIbbaQB8mBLrJWASFaGdXPs3cNA8Q?e=ekSAkf

@nikolaredstork nikolaredstork requested a review from flomnes April 2, 2025 07:24
Copy link

@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.

This looks very nice, a few minor remarks

  • Rename reservoir levels => rule curves
  • Place all loading logic into the loader class implementations (single / scenarized)

Thanks !

ret = area.hydro.series->saveToFolder(area.id, buffer, hydroPmax) && ret;

buffer.clear() << folder << SEP << "input" << SEP << "hydro";
ret = area.hydro.series->reservoirLevels.saveToFolder(area.id, buffer) && ret;
Copy link

Choose a reason for hiding this comment

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

Suggested change
ret = area.hydro.series->reservoirLevels.saveToFolder(area.id, buffer) && ret;
ret = area.hydro.series->ruleCurves.saveToFolder(area.id, buffer) && ret;

Copy link
Author

@nikolaredstork nikolaredstork May 23, 2025

Choose a reason for hiding this comment

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

Regarding naming convention, from my point of view, it is natural to stay as it is, since in the Antares documentation we called it reservoir levels. If new Antares Simulator developer starts to work, he would search for object called reservoirLevels, not ruleCurves. Nevertheless, if you don't agree please tell me, It won't be hard to change it. Thanks.

"Value not supported for study.parameters.compatibility.hydroPmax");
}

ret = area.hydro.series->reservoirLevels.loadReservoirLevels(
Copy link

Choose a reason for hiding this comment

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

Suggested change
ret = area.hydro.series->reservoirLevels.loadReservoirLevels(
ret = area.hydro.series->ruleCurves.loadFromFolder(

Copy link
Author

Choose a reason for hiding this comment

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

Done!

// This functionality will no longer be required once the UI is fully deprecated.
Matrix<double> standardReservoirLevelMatrix;

TimeSeriesNumbers& timeseriesNumbers;
Copy link

Choose a reason for hiding this comment

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

Place this member before TimeSeries members for initialization reasons

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Comment on lines +83 to +84
standardReservoirLevelMatrix.fillColumn(ReservoirLevels::maximum, 1.);
standardReservoirLevelMatrix.fillColumn(ReservoirLevels::average, 0.5);
Copy link

Choose a reason for hiding this comment

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

If standardReservoirLevelMatrix is only required for the GUI, add ui or GUI in it's name

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Comment on lines +106 to +110
bool ReservoirLevels::loadReservoirLevels(
const std::string& areaID,
const std::filesystem::path& folder,
bool usedBySolver,
Parameters::Compatibility::HydroRuleCurves hydroRuleCurves)
Copy link

Choose a reason for hiding this comment

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

Isn't there a ReservoirLevelsLoader class ? If so, why does ReservoirLevels still has some code related to loading ?

Copy link
Author

Choose a reason for hiding this comment

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

I've created RuleCurvesLoaderService class to support this functionality. Please check it out, and if you have additional comments, please add it.

Comment on lines +62 to +63
//! check reservoir levels
bool checkReservoirLevels(uint year);
Copy link

Choose a reason for hiding this comment

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

rule curves

Copy link
Author

Choose a reason for hiding this comment

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

Done!

{
prepareInflows_.Run(year);
minGenerationScaling_.Run(year);
if (!checkReservoirLevels(year))
Copy link

Choose a reason for hiding this comment

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

rule curves

Copy link

@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.

Also, do you use the existing hydro prefix for scenariobuilder.dat ?

@flomnes Since we are using the same object for storing TimeSeriesNumbers, we are using the same scenario builder, so the answer is yes.

@flomnes
Copy link

flomnes commented May 22, 2025

Finally, could you point to our repository instead of RTE-i's fork ?

@nikolaredstork nikolaredstork force-pushed the feature/multiple-rule-curve-cr-24 branch 3 times, most recently from e8ceb21 to 897a4ec Compare May 29, 2025 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants