[TEST] Refactoring study's loader #893

kathvargasr · 2022-10-06T07:47:42Z

No description provided.

flomnes

Function Study::internalLoadFromFolder is still way too long, consider cutting it into smaller functions

src/libs/antares/study/load-params.cpp

src/libs/antares/study/study.cpp

src/tests/src/libs/antares/study/loader/CMakeLists.txt

src/tests/src/libs/antares/study/loader/test-study-load-internal.cpp

sonarqubecloud · 2022-10-27T10:16:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
34 Code Smells

0.0% Coverage
0.0% Duplication

flomnes · 2022-10-28T16:15:26Z

src/tests/src/libs/antares/study/CMakeLists.txt

+add_subdirectory(scenario-builder)
+add_subdirectory(loader)


study-loader

flomnes · 2022-10-28T16:28:49Z

src/tests/src/libs/antares/study/wrappers/study-wrapper.h

+    void initializeSetsData(){
+        pstudy->initializeSetsData();
+    }


Added for tests, not tested though. Either remove from this attorney class, or test.

flomnes · 2022-10-28T16:29:02Z

src/tests/src/libs/antares/study/wrappers/study-wrapper.h

+    void inputExtensionCompatibility(){
+        pstudy->inputExtensionCompatibility();
+    }
+    //! Release all unnecessary buffers
+    void reduceMemoryUsage(){
+        pstudy->reduceMemoryUsage();
+    }
+    //@}


Same as above

flomnes · 2022-10-28T16:29:14Z

src/tests/src/libs/antares/study/wrappers/study-wrapper.h

+#include "runtime.h"
+


Include at top ? Or don't include

flomnes · 2022-10-28T16:37:39Z

src/tests/src/libs/antares/study/wrappers/study-wrapper.h

+** \brief Antares StudyWrapper
+*/
+
+using pStudy = std::shared_ptr<Study>;


Study::Ptr is already an alias to shared_ptr<Study>

And pStudy is very often (if not every time) a class data member variable that contains an instance of Study, not a type as here. There can be a confusion.

flomnes · 2022-10-28T16:38:55Z

src/tests/src/libs/antares/study/wrappers/study-wrapper.h

+{
+
+public:
+    using Ptr = std::shared_ptr<StudyWrapper>;


Not needed in my opinion. This object can be allocated on the stack

flomnes · 2022-10-28T16:40:43Z

src/tests/src/libs/antares/study/wrappers/study-wrapper.h

+#include <study.h>
+


study-wrapper.h isn't part of any CMakeFile. It may result in an incomplete dependency graph.

guilpier-code · 2022-11-14T14:30:52Z

Why not leave a clear introduction to the work ? Ok, I often forget to leave one but it's still useful

Caution : the comments I leave here are not necessarily mandatory, we should discuss them

guilpier-code · 2022-11-14T14:55:13Z

src/libs/antares/study/study.cpp

-    if (!nbLogicalCores)
-        logs.fatal() << "Number of logical cores available is 0.";
+
+    table[ncMin] = 1;


We should rename table into number_of_MC_year_threads (for instance), table is meaningless.
Besides, uint in std::map<NumberOfCoresMode, uint> is overstated. int is enough.

guilpier-code · 2022-11-14T15:13:42Z

src/libs/antares/study/study.cpp

+    computeRawNbParallelYear(options.forceParallel, options.maxNbYearsInParallel);
+    maxNbYearsInParallel = getNbYearsParallelRaw();


Why not :

maxNbYearsInParallel = computeRawNbParallelYear(options.forceParallel, options.maxNbYearsInParallel);

?

guilpier-code · 2022-11-14T15:54:41Z

src/tests/src/libs/antares/study/wrappers/study-wrapper.h

+** \brief Antares StudyWrapper
+*/
+
+using pStudy = std::shared_ptr<Study>;


And pStudy is very often (if not every time) a class data member variable that contains an instance of Study, not a type as here. There can be a confusion.

guilpier-code · 2022-11-14T16:07:15Z

src/tests/src/libs/antares/study/wrappers/study-wrapper.h

+*/
+
+using pStudy = std::shared_ptr<Study>;
+class StudyWrapper


What is the point of this study wrapper ? (It may have a real benefit, but I can't see which one)

I can see that this wrapper is only used (for now) in the newly created tests. So, if it is meant to be used in the context of tests, maybe we should move it next to the tests.

guilpier-code · 2022-11-14T16:35:02Z

src/libs/antares/study/study.cpp

@@ -48,6 +48,8 @@
 #include <yuni/core/system/cpu.h> // For use of Yuni::System::CPU::Count()
 #include <math.h>                 // For use of floor(...) and ceil(...)

+#include <string>


#include <string> : is it useful ?

guilpier-code · 2022-11-14T17:19:06Z

src/libs/antares/study/study.cpp

@@ -320,151 +322,127 @@ void Study::ensureDataAreAllInitialized()
    StudyEnsureDataThermalPrepro(this);
 }

-std::map<std::string, uint> Study::getRawNumberCoresPerLevel()
+
+void Study::computeRawNbParallelYear(const bool forceParallel, const uint nbYearsParallelForced)


In this file, all changes are related to parallel MC years, right ?
We could try to extract all the logic (computing sets of parallel years) from the study, in order to :

empty a little bit the god class Study. Said in another way, it would the number of responsibilities of Study

this extracted code could be tested in isolation.

avoid currently existing code duplication with solver.hxx : buildSetsOfParallelYears(...)

We could have something like :

class setsOfParallelYearsCalculator // Or some name more accurate { public: setsOfParallelYearsCalculator ( int nbYears, bool userPlaylist, int timeSeriesToGenerate, int timeSeriesToRefresh, bool forceParallel, int forcedNbOfParallelYears // Today called "maxNbYearsInParallel" ); void build(); // Main function, it would do what "computeNumberOfCores" does bool allSetsParallelYearsHaveSameSize(); private: // Private methods computeRawNbParallelYear(); computeMinNbYearsInParallelYearSet(); // Private data members std::vector<std::vector<uint>> setsOfParallelYears_; }

Note :

In this proposal, the class does not take the whole object parameters as an argument for its constructor. Instead, we would pass the constructor exactly the list of data it really needs. Indeed, class parameters can be split or can change. The data we give won't.

guilpier-code · 2022-11-15T10:49:01Z

src/libs/antares/study/load-params.cpp

+        set.clear();
+    }
+
+    uint size(Study::SingleSetOfAreas& set) const


uint size(Study::SingleSetOfAreas& set) const : I don't see the point of this function.
Instead of calling sets_of_areas_handler.size(set_of_areas), let's simply call sets_of_areas.size()

guilpier-code · 2022-11-15T12:49:23Z

src/libs/antares/study/load-params.cpp

+
+}
+
+class SetHandlerAreas


Each method of class SetHandlerAreas is given a SingleSetOfAreas& set (which is a std::set<Area*>), and makes something with this argument.
Why not put the set in the class and rename the class setOfAreas ?
There would be some side effects to that change : it will bring some other changes in the code.
It remains to evaluate the size of these changes.

guilpier-code · 2022-11-15T13:19:35Z

src/libs/antares/study/load-params.cpp

+namespace Data
+{
+
+bool Study::initializeInternalData(const StudyLoadOptions& options)


Apparently, method initializeInternalData does 3 (at least) things :

it initializes parameters (it should be done by a method of class Parameters, or better : in the Parameters constructor, if possible)

it initializes the calendar (same remark as above, with class Date::Calendar)

it makes a check (should be moved with other checks : to be explored)

Each bullet should be separated from the others in a function / method
This split would have few consequences test-study-load-internal.cpp.

kathvargasr · 2023-01-05T09:57:20Z

replaced by #1017 and #1057

kathvargasr added the work in progress label Oct 6, 2022

pull-request-size bot added the size/L label Oct 6, 2022

kathvargasr force-pushed the feature/test-study-refactoring branch from 57f8f1f to f2e79db Compare October 10, 2022 08:36

pull-request-size bot added size/XL and removed size/L labels Oct 10, 2022

kathvargasr force-pushed the feature/test-study-refactoring branch 4 times, most recently from ab33615 to 07da5d9 Compare October 25, 2022 09:33

[TEST] Refactoring study's loader

c166bb2

kathvargasr force-pushed the feature/test-study-refactoring branch from 07da5d9 to c166bb2 Compare October 25, 2022 09:53

kathvargasr added 2 commits October 25, 2022 14:24

[TEST] Fix test-study-load-internal

39d818c

Move wrappers to test folder

ac9cb47

kathvargasr marked this pull request as ready for review October 26, 2022 07:13

kathvargasr requested review from flomnes and guilpier-code October 26, 2022 07:13

flomnes requested changes Oct 26, 2022

View reviewed changes

Resolve requested changes

0897afd

kathvargasr force-pushed the feature/test-study-refactoring branch from 9a6db98 to 0897afd Compare October 26, 2022 15:02

Resolving requested changes pt2

28bfd0a

kathvargasr requested a review from flomnes October 27, 2022 11:49

kathvargasr added tests and removed work in progress labels Oct 27, 2022

flomnes requested changes Oct 28, 2022

View reviewed changes

guilpier-code requested changes Nov 15, 2022

View reviewed changes

kathvargasr closed this Jan 5, 2023

kathvargasr deleted the feature/test-study-refactoring branch January 5, 2023 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEST] Refactoring study's loader #893

[TEST] Refactoring study's loader #893

kathvargasr commented Oct 6, 2022

flomnes left a comment

sonarqubecloud bot commented Oct 27, 2022

flomnes Oct 28, 2022

flomnes Oct 28, 2022

flomnes Oct 28, 2022

flomnes Oct 28, 2022

flomnes Oct 28, 2022

guilpier-code Nov 14, 2022

flomnes Oct 28, 2022

flomnes Oct 28, 2022

guilpier-code commented Nov 14, 2022 •

edited

Loading

guilpier-code Nov 14, 2022

guilpier-code Nov 14, 2022

guilpier-code Nov 14, 2022

guilpier-code Nov 14, 2022

guilpier-code Nov 14, 2022

guilpier-code Nov 14, 2022

guilpier-code Nov 15, 2022

guilpier-code Nov 15, 2022

guilpier-code Nov 15, 2022

kathvargasr commented Jan 5, 2023

		computeRawNbParallelYear(options.forceParallel, options.maxNbYearsInParallel);
		maxNbYearsInParallel = getNbYearsParallelRaw();

		add_subdirectory(scenario-builder)
		add_subdirectory(loader)

[TEST] Refactoring study's loader #893

[TEST] Refactoring study's loader #893

Conversation

kathvargasr commented Oct 6, 2022

flomnes left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 27, 2022

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

guilpier-code commented Nov 14, 2022 • edited Loading

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

kathvargasr commented Jan 5, 2023

guilpier-code commented Nov 14, 2022 •

edited

Loading