Thermal clusters lists : cleaning #1844

guilpier-code · 2023-12-22T16:27:45Z

This PR follows the PR #1809 (about cleaning common cluster list and renewable cluster list)
It aims at simplifying thermal cluster lists as much as possible.

Goal :
Having only one list of thermal clusters.
This list would be private
Code would interact with this list through an interface. This interface would contain functions returning std::views that would result in filtering thermal clusters using a certain property of clusters : enabled / disabled, must-run or not, a combination of these.

What's done :

Simplifying the ClusterList<T>::add() method
Changing std::map mapping from class ThermalClusterList into a std::vector (named allClusters_)
Removing PartThermal::clusters (often reached by area.thermal.clusters)
Removing PartThermal::mustrunList (often reached by area.thermal.mustrunList)
Removing PartThermal::list (often reached by area.thermal.list.clusters)

Before starting a review, one should know several things :

what the old lists of clusters used to contain :
- area.thermal.list.mapping : all clusters
- area.thermal.mustrunList.clusters : clusters must-run and enabled
- area.thermal.clusters : enabled clusters
- area.thermal.list.clusters : list used to build the optimization problem (clusters enabled and not must-run). Careful with that list : when loading clusters from input, this list contains all clusters. Then it is progressively cleared from disabled and must-run clusters. So its content depends on where we are in the execution. This list is also used from the GUI. In that case, it always contains all clusters : it is not cleared from specific clusters.
the list area.thermal.list.clusters is the only list used in the GUI, and in that case, it contains all clusters.
As said previously, all lists of clusters are removed, except area.thermal.list.mapping, turned into a private std::vector (area.thermal.list.allClusters_) containing all clusters, and filtered to access only clusters of interest at any point of the code.
cluster indices : any cluster (thermal / renewable) used to have 2 indices :
- index : it turns out to be an index regarding only enabled and not must-run clusters. So this index never concerns renewable clusters. It was moved down from parent class Cluster to class ThermalCluster.
- areaWideIndex : index regarding only enabled clusters. Concerns both thermal and renewable clusters
the 2 lists area.thermal.mustrunList.clusters and area.thermal.list.clusters are closely related (inter-dependent) to the renewable cluster list, due to a common inheritance. So this work deeply impacts the renewable cluster lists.

This PR handles a part of issue #1487 (simplify and clarify cluster lists) : Co-authored-by: Guillaume PIERRE <[email protected]>

…ass) and consequences

watermelon-copilot-for-code-review · 2023-12-22T16:27:50Z

Watermelon AI Summary

AI Summary deactivated by guilpier-code

GitHub PRs

#1841 - Add tests on short-term storage/thermal cluster/renewable cluster removal - By flomnes 1 weeks ago
#1871 - fix(study-factory): ignore non-existent files in archived studies during build - By laurent-laporte-pro 1 weeks ago
#1844 - Thermal clusters lists : cleaning - By guilpier-code 1 weeks ago

Antares_Simulator is an open repo and Watermelon will serve it for free.
🍉🫶

…s, we remove that update

…thermal_lists

… functions

src/libs/antares/study/runtime/runtime.cpp

flomnes · 2024-01-17T10:53:28Z

src/libs/antares/study/runtime/runtime.cpp

-              thermalTSRefresh
-                = thermalTSRefresh || cluster.doWeGenerateTS(globalThermalTSgeneration);
-          });
+        for (auto c : area.thermal.list.each_enabled_and_not_mustrun())


Some redundancy. In some cases, we want to iterate over a container. In other cases, we want the number of elements.

It would be nicer to have

for (auto c : thermal.list.enabledNotMustRun.each()) { // ... } auto nbClusters = thermal.list.enabledNotMustRun.count();

Also, clear

thermal.list.enabledNotMustRun.clear();

So what you suggest is several lists, right ?
If so, it's goes in the opposite direction of the current work, which is to have only one list that we can filter.

flomnes · 2024-01-17T10:54:02Z

src/libs/antares/study/runtime/runtime.h

@@ -136,8 +136,6 @@ class StudyRuntimeInfos
    void initializeRangeLimits(const Study& study, StudyRangeLimits& limits);
    //! Prepare all thermal clusters in 'must-run' mode
    void initializeThermalClustersInMustRunMode(Study& study) const;
-    void removeDisabledThermalClustersFromSolverComputations(Study& study);
-    void removeDisabledRenewableClustersFromSolverComputations(Study& study);
    void removeDisabledShortTermStorageClustersFromSolverComputations(Study& study);


What do you mean ?

src/libs/antares/study/parts/renewable/cluster_list.cpp

guilpier-code · 2024-01-18T10:49:08Z

src/ui/action/handler/antares-study/area/create.cpp

@@ -293,7 +293,7 @@ void Create::createActionsForAStandardAreaCopy(Context& ctx, bool copyPosition)
        auto* root = new RootNodePlant(pOriginalAreaName);

        // browsing each thermal cluster
-        for (auto& c : area->thermal.list)


About :

for (auto& c : area->thermal.list)

Actually, area->thermal.list.clusters was traversed here, because of method begin() and end() (now gone) were used behind the scene.

guilpier-code · 2024-01-18T10:54:06Z

src/libs/antares/study/parts/common/cluster.h

@@ -24,37 +24,21 @@ struct CompareClusterName final
 class Cluster
 {
 public:
-    //! Map of renewable clusters
-    using Map = std::map<ClusterName, Cluster*>;
+    using Set = std::set<Cluster*, CompareClusterName>;


using Set = std::set<Cluster*, CompareClusterName>; : only moved from the bottom to the top of the class

guilpier-code · 2024-01-18T10:58:40Z

src/libs/antares/study/parts/common/cluster_list.h

-    ** \brief Iterate through all clusters (const)
-    */
-    template<class PredicateT>
-    void each(const PredicateT& predicate) const


void each(const PredicateT& predicate) const :
each(...) was used to traverse list clusters (clusters enabled and not must-run) and process a task on each element. We don't need it anymore : it was often replaced with a loop over the filtered complete list.

guilpier-code · 2024-01-18T12:33:41Z

src/libs/antares/study/parts/common/cluster_list.h

-    ** \param t The cluster to add
-    ** \return True if the cluster has been added, false otherwise
-    */
-    SharedPtr add(const SharedPtr clusters);


SharedPtr add(const SharedPtr clusters); :
is replace with :
void addToCompleteList(std::shared_ptr<ClusterT> cluster);

guilpier-code · 2024-01-18T12:42:20Z

src/libs/antares/study/parts/common/cluster_list.h

    **
    ** \param id ID of the cluster to find
    ** \return A pointer to a cluster. nullptr if not found
    */
-    ClusterT* find(const Data::ClusterName& id) const;


ClusterT* find(const Data::ClusterName& id) const; :
Used to be called only in GUI, searches in list clusters.
In GUI, clusters was containing all clusters.
It's been renamed findInAll , and now searches in private list allClusters_

guilpier-code · 2024-01-19T09:31:48Z

src/libs/antares/study/scenario-builder/RenewableTSNumberData.cpp

@@ -32,17 +32,15 @@ bool renewableTSNumberData::apply(Study& study)

    const uint tsGenCountRenewable = get_tsGenCount(study);

-    uint clusterIndex = 0;


For renewable clusters, area.renewable.list used to contain (at this point of execution) only enabled clusters.
That's why we now loop over area.renewable.list.each_enabled()

guilpier-code · 2024-01-19T09:33:18Z

src/libs/antares/study/scenario-builder/RenewableTSNumberData.cpp

@@ -66,25 +64,16 @@ void renewableTSNumberData::saveToINIFile(const Study& /* study */,
    if (!pArea)
        return;

-// Foreach year
-#ifndef NDEBUG


#ifndef NDEBUG .. #endif

was remove, unnecessary / too few benefit

guilpier-code · 2024-01-19T09:34:54Z

src/libs/antares/study/scenario-builder/RenewableTSNumberData.cpp

@@ -99,7 +88,7 @@ bool renewableTSNumberData::reset(const Study& study)
    //   solver or not.
    // WARNING: At this point in time, the variable pArea->renewable.clusterCount()
    //   might not be valid (because not really initialized yet)
-    const uint clusterCount = pArea->renewable.list.size();


reset() can be called both from GUI and solver. In GUI, the cluster list used contains contains all clusters. Otherwise, only enabled clusters. Setting clusterCount with allClustersCount() ensures that the matrix (of TS number) reset with clusterCount is large enough, as it is a max.

guilpier-code · 2024-01-19T10:01:55Z

src/libs/antares/study/scenario-builder/ThermalTSNumberData.cpp

@@ -15,11 +15,11 @@ bool thermalTSNumberData::reset(const Study& study)
    // If an area is available, it can only be an overlay for thermal timeseries
    // WARNING: The total number of clusters may vary if used from the
    //   solver or not.
-    // WARNING: At this point in time, the variable pArea->thermal.clusterCount()


In this file, the cluster lists usage used to be and still is particularly tricky.
What I mean :
the general philosophy in the scenario builder is :

if a function of this file is called from GUI, the clusters list it processes contains all clusters

if a function of this file is called from solver it contains only enabled clusters (must-run or not)

For example :

reset() is called from load higher level functions, so it can be called from both solver and GUI. This why we have a ternary operator inside this function. But the code before changes is wrong : when loading, the list pArea->thermal.list.clusters contains all cluster. So size pArea->thermal.list.size() + pArea->thermal.mustrunList.size() is too large. New code is both more clear and correct.

apply() : this function is only used by the solver. In the old code, it processes list area.thermal.clusters, that contains enabled thermal clusters (so it confirms that the intention is to process enabled clusters only in the scenario builder on the solver side). That's why each_enabled() is called in the new code.

saveToINIFile(...) : this function is only called from the GUI. In the old code, it processes pArea->thermal.list, which (when using the GUI) contains all clusters. New code is much more clear about that.

guilpier-code · 2024-01-19T13:06:48Z

src/solver/simulation/solver.hxx

@@ -627,7 +627,7 @@ void ISimulation<Impl>::allocateMemoryForRandomNumbers(randomNumbers& randomForP
        {
            // logs.info() << "   area : " << a << " :";
            auto& area = *(study.areas.byIndex[a]);
-            size_t nbClusters = area.thermal.list.mapping.size();


Recall that area.thermal.list.mapping used to contain all thermal clusters

src/libs/antares/study/parts/thermal/cluster_list.h

…from functions

sonarqubecloud · 2024-02-19T15:39:55Z

Quality Gate passed

Issues
12 New issues

Measures
0 Security Hotspots
17.9% Coverage on New Code
2.6% Duplication on New Code

See analysis details on SonarCloud

payetvin and others added 10 commits December 4, 2023 17:47

[DEV] remove cluster in renewable container

8d3a971

[DEV] use vector for byIndex

3a1202c

[DEV] remove uselss renewlist constructor

ff6795d

[FIX] segfault

947b5ff

[DEV] Code smells

8dfc03f

[CHERRY-PICK] enable cpp 20

206f45e

Merge remote-tracking branch 'github/develop' into HEAD

aacb6b1

Revert faulty YML files

a4cdc1c

Refactor common ClusterList class (#1813)

fbf0ade

This PR handles a part of issue #1487 (simplify and clarify cluster lists) : Co-authored-by: Guillaume PIERRE <[email protected]>

Clean thermal lists : simplifying added(...) function (common list cl…

ee1e5f5

…ass) and consequences

guilpier-code added cleaning code quality labels Dec 22, 2023

guilpier-code added this to the v8.8.1 milestone Dec 22, 2023

guilpier-code requested review from sylvlecl, flomnes and payetvin December 22, 2023 16:27

pull-request-size bot added the size/M label Dec 22, 2023

Clean thermal lists : removing use of mapping in a unit test

7c7b6ea

pull-request-size bot added size/L and removed size/M labels Dec 26, 2023

guilpier-code force-pushed the fix/clean_thermal_lists branch from d022bb6 to 7c7b6ea Compare December 27, 2023 10:33

pull-request-size bot added size/M and removed size/L labels Dec 27, 2023

Base automatically changed from fix/renew-lists to develop December 27, 2023 13:06

pull-request-size bot added size/XXL and removed size/M labels Dec 27, 2023

guilpier-code added 2 commits December 27, 2023 17:45

Clean thermal lists : update PartThermal::clusters from GUI is useles…

8e3ba46

…s, we remove that update

Merge remote-tracking branch 'remotes/origin/develop' into fix/clean_…

eceedd3

…thermal_lists

pull-request-size bot removed the size/XXL label Dec 28, 2023

guilpier-code added 3 commits January 15, 2024 14:32

Clean thermal lists : completely remove list.clusters_

e5a9ebb

Clean thermal lists : fix a compilation error

242ba0f

Clean cluster lists : make giveIndicesToClusters() a protected member…

119746e

… functions

flomnes reviewed Jan 17, 2024

View reviewed changes

src/libs/antares/study/runtime/runtime.cpp Outdated Show resolved Hide resolved

flomnes reviewed Jan 17, 2024

View reviewed changes

guilpier-code added 2 commits January 18, 2024 17:37

Clean thermal lists : correction of an error

9c321dd

Clean thermal lists : correcting an error

beecabc

guilpier-code commented Jan 19, 2024

View reviewed changes

guilpier-code marked this pull request as ready for review January 19, 2024 13:56

flomnes reviewed Jan 29, 2024

View reviewed changes

src/libs/antares/study/parts/thermal/cluster_list.h Outdated Show resolved Hide resolved

payetvin added 12 commits February 15, 2024 16:08

renamed giveIndicesToClusters

fcc3ee8

remove isNotMustRun, remove assert not compiling, remove unused args …

08b28c7

…from functions

rename enabledAndMustRunCount

d2c6f3b

activate sonarcloud

3689c46

merge develop, conflicts

f0d7236

fix compile

00a911b

use all() to reset TS

4c47d7d

code smells 1

f3f73c1

code smells 2

3790f82

update test_study to account for new enabled behavior

973572e

fix compile

8e80430

code smells 3

59d249f

payetvin modified the milestones: v8.8.1, Sprint 2 Feb 19, 2024

code smells 4

106db37

payetvin approved these changes Feb 19, 2024

View reviewed changes

remove double sonar job

0047102

JasonMarechal25 merged commit b181904 into develop Feb 22, 2024
5 checks passed

JasonMarechal25 deleted the fix/clean_thermal_lists branch February 22, 2024 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thermal clusters lists : cleaning #1844

Thermal clusters lists : cleaning #1844

guilpier-code commented Dec 22, 2023 •

edited

Loading

watermelon-copilot-for-code-review bot commented Dec 22, 2023 •

edited

Loading

flomnes Jan 17, 2024

guilpier-code Jan 19, 2024

flomnes Jan 17, 2024

guilpier-code Jan 19, 2024

guilpier-code Jan 18, 2024

guilpier-code Jan 18, 2024

guilpier-code Jan 18, 2024

guilpier-code Jan 18, 2024

guilpier-code Jan 18, 2024

guilpier-code Jan 19, 2024

guilpier-code Jan 19, 2024

guilpier-code Jan 19, 2024

guilpier-code Jan 19, 2024

guilpier-code Jan 19, 2024

sonarqubecloud bot commented Feb 19, 2024

		@@ -32,17 +32,15 @@ bool renewableTSNumberData::apply(Study& study)

		const uint tsGenCountRenewable = get_tsGenCount(study);

		uint clusterIndex = 0;

Thermal clusters lists : cleaning #1844

Thermal clusters lists : cleaning #1844

Conversation

guilpier-code commented Dec 22, 2023 • edited Loading

watermelon-copilot-for-code-review bot commented Dec 22, 2023 • edited Loading

Watermelon AI Summary

GitHub PRs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 19, 2024

Quality Gate passed

guilpier-code commented Dec 22, 2023 •

edited

Loading

watermelon-copilot-for-code-review bot commented Dec 22, 2023 •

edited

Loading