Skip to content

Commit 6bbd630

Browse files
figueroa1395figueroa1395
authored
Merge pull request #1122 from PowerGridModel/feature/remove-logger-from-adapter
Clean-up main model: Pass logger by reference to adapter
2 parents 375068d + 3999181 commit 6bbd630

File tree

4 files changed

+32
-96
lines changed

4 files changed

+32
-96
lines changed

‎power_grid_model_c/power_grid_model/include/power_grid_model/job_adapter.hpp‎

Lines changed: 8 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
#include "main_model_fwd.hpp"
1111

1212
#include "auxiliary/dataset.hpp"
13-
#include "common/dummy_logging.hpp"
1413
#include "main_core/update.hpp"
1514

1615
namespace power_grid_model {
@@ -23,19 +22,15 @@ template <class MainModel> class JobAdapter : public JobInterface<JobAdapter<Mai
2322

2423
JobAdapter(std::reference_wrapper<MainModel> model_reference,
2524
std::reference_wrapper<MainModelOptions const> options)
26-
: model_reference_{model_reference}, options_{options} {
27-
reset_logger_impl();
28-
}
25+
: model_reference_{model_reference}, options_{options} {}
2926
JobAdapter(JobAdapter const& other)
3027
: model_copy_{std::make_unique<MainModel>(other.model_reference_.get())},
3128
model_reference_{std::ref(*model_copy_)},
3229
options_{std::ref(other.options_)},
3330
components_to_update_{other.components_to_update_},
3431
update_independence_{other.update_independence_},
3532
independence_flags_{other.independence_flags_},
36-
all_scenarios_sequence_{other.all_scenarios_sequence_} {
37-
reset_logger_impl();
38-
}
33+
all_scenarios_sequence_{other.all_scenarios_sequence_} {}
3934
JobAdapter& operator=(JobAdapter const& other) {
4035
if (this != &other) {
4136
model_copy_ = std::make_unique<MainModel>(other.model_reference_.get());
@@ -45,8 +40,6 @@ template <class MainModel> class JobAdapter : public JobInterface<JobAdapter<Mai
4540
update_independence_ = other.update_independence_;
4641
independence_flags_ = other.independence_flags_;
4742
all_scenarios_sequence_ = other.all_scenarios_sequence_;
48-
49-
reset_logger_impl();
5043
}
5144
return *this;
5245
}
@@ -57,9 +50,7 @@ template <class MainModel> class JobAdapter : public JobInterface<JobAdapter<Mai
5750
components_to_update_{std::move(other.components_to_update_)},
5851
update_independence_{std::move(other.update_independence_)},
5952
independence_flags_{std::move(other.independence_flags_)},
60-
all_scenarios_sequence_{std::move(other.all_scenarios_sequence_)} {
61-
reset_logger_impl();
62-
}
53+
all_scenarios_sequence_{std::move(other.all_scenarios_sequence_)} {}
6354
JobAdapter& operator=(JobAdapter&& other) noexcept {
6455
if (this != &other) {
6556
model_copy_ = std::move(other.model_copy_);
@@ -69,15 +60,10 @@ template <class MainModel> class JobAdapter : public JobInterface<JobAdapter<Mai
6960
update_independence_ = std::move(other.update_independence_);
7061
independence_flags_ = std::move(other.independence_flags_);
7162
all_scenarios_sequence_ = std::move(other.all_scenarios_sequence_);
72-
73-
reset_logger_impl();
7463
}
7564
return *this;
7665
}
77-
~JobAdapter() {
78-
reset_logger_impl();
79-
model_copy_.reset();
80-
}
66+
~JobAdapter() { model_copy_.reset(); }
8167

8268
private:
8369
// Grant the CRTP base (JobInterface<JobAdapter>) access to
@@ -96,14 +82,12 @@ template <class MainModel> class JobAdapter : public JobInterface<JobAdapter<Mai
9682
// current_scenario_sequence_cache_ is calculated per scenario, so it is excluded from the constructors.
9783
typename ModelType::SequenceIdx current_scenario_sequence_cache_{};
9884

99-
Logger* log_{nullptr};
100-
101-
void calculate_impl(MutableDataset const& result_data, Idx scenario_idx) const {
85+
void calculate_impl(MutableDataset const& result_data, Idx scenario_idx, Logger& logger) const {
10286
MainModel::calculator(options_.get(), model_reference_.get(), result_data.get_individual_scenario(scenario_idx),
103-
false, logger());
87+
false, logger);
10488
}
10589

106-
void cache_calculate_impl() const {
90+
void cache_calculate_impl(Logger& logger) const {
10791
// calculate once to cache topology, ignore results, all math solvers are initialized
10892
try {
10993
MainModel::calculator(options_.get(), model_reference_.get(),
@@ -113,7 +97,7 @@ template <class MainModel> class JobAdapter : public JobInterface<JobAdapter<Mai
11397
"sym_output",
11498
model_reference_.get().meta_data(),
11599
},
116-
true, logger());
100+
true, logger);
117101
} catch (SparseMatrixError const&) { // NOLINT(bugprone-empty-catch) // NOSONAR
118102
// missing entries are provided in the update data
119103
} catch (NotObservableError const&) { // NOLINT(bugprone-empty-catch) // NOSONAR
@@ -158,13 +142,5 @@ template <class MainModel> class JobAdapter : public JobInterface<JobAdapter<Mai
158142
return std::span<Idx2D const>{std::get<comp_idx>(current_scenario_sequence_cache_)};
159143
});
160144
}
161-
162-
void reset_logger_impl() { log_ = nullptr; }
163-
void set_logger_impl(Logger& log) { log_ = &log; }
164-
165-
Logger& logger() const {
166-
static common::logging::NoLogger no_log{};
167-
return log_ != nullptr ? *log_ : no_log;
168-
}
169145
};
170146
} // namespace power_grid_model

‎power_grid_model_c/power_grid_model/include/power_grid_model/job_dispatch.hpp‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,8 @@ class JobDispatch {
2424
static BatchParameter batch_calculation(Adapter& adapter, ResultDataset const& result_data,
2525
UpdateDataset const& update_data, Idx threading,
2626
common::logging::MultiThreadedLogger& log) {
27-
adapter.set_logger(log);
28-
2927
if (update_data.empty()) {
30-
adapter.calculate(result_data);
31-
adapter.reset_logger();
28+
adapter.calculate(result_data, log);
3229
return BatchParameter{};
3330
}
3431

@@ -38,12 +35,11 @@ class JobDispatch {
3835
// if the batch_size is zero, it is a special case without doing any calculations at all
3936
// we consider in this case the batch set is independent but not topology cacheable
4037
if (n_scenarios == 0) {
41-
adapter.reset_logger();
4238
return BatchParameter{};
4339
}
4440

4541
// calculate once to cache, ignore results
46-
adapter.cache_calculate();
42+
adapter.cache_calculate(log);
4743

4844
// error messages
4945
std::vector<std::string> exceptions(n_scenarios, "");
@@ -55,8 +51,6 @@ class JobDispatch {
5551

5652
handle_batch_exceptions(exceptions);
5753

58-
adapter.reset_logger();
59-
6054
return BatchParameter{};
6155
}
6256

@@ -92,7 +86,6 @@ class JobDispatch {
9286
auto const copy_adapter_functor = [&base_adapter, &thread_log]() {
9387
Timer const t_copy_adapter_functor{thread_log, LogEvent::copy_model};
9488
auto result = Adapter{base_adapter};
95-
result.set_logger(thread_log);
9689
return result;
9790
};
9891

@@ -114,7 +107,9 @@ class JobDispatch {
114107
adapter = copy_adapter_functor();
115108
};
116109

117-
auto run = [&adapter, &result_data](Idx scenario_idx) { adapter.calculate(result_data, scenario_idx); };
110+
auto run = [&adapter, &result_data, &thread_log](Idx scenario_idx) {
111+
adapter.calculate(result_data, scenario_idx, thread_log);
112+
};
118113

119114
auto calculate_scenario = JobDispatch::call_with<Idx>(std::move(run), std::move(setup), std::move(winddown),
120115
JobDispatch::scenario_exception_handler(exceptions),

‎power_grid_model_c/power_grid_model/include/power_grid_model/job_interface.hpp‎

Lines changed: 12 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,23 @@ template <typename Adapter> class JobInterface {
1818
public:
1919
// the multiple NOSONARs are used to avoid the complaints about the unnamed concepts
2020
template <typename ResultDataset>
21-
void calculate(ResultDataset const& result_data, Idx pos = 0)
21+
void calculate(ResultDataset const& result_data, Idx pos, Logger& logger)
2222
requires requires(Adapter& adapter) { // NOSONAR
23-
{ adapter.calculate_impl(result_data, pos) } -> std::same_as<void>;
23+
{ adapter.calculate_impl(result_data, pos, logger) } -> std::same_as<void>;
2424
}
2525
{
26-
return static_cast<Adapter*>(this)->calculate_impl(result_data, pos);
26+
static_cast<Adapter*>(this)->calculate_impl(result_data, pos, logger);
27+
}
28+
template <typename ResultDataset> void calculate(ResultDataset const& result_data, Logger& logger) {
29+
calculate(result_data, Idx{}, logger);
2730
}
2831

29-
void cache_calculate()
32+
void cache_calculate(Logger& logger)
3033
requires requires(Adapter& adapter) { // NOSONAR
31-
{ adapter.cache_calculate_impl() } -> std::same_as<void>;
34+
{ adapter.cache_calculate_impl(logger) } -> std::same_as<void>;
3235
}
3336
{
34-
return static_cast<Adapter*>(this)->cache_calculate_impl();
37+
static_cast<Adapter*>(this)->cache_calculate_impl(logger);
3538
}
3639

3740
template <typename UpdateDataset>
@@ -40,7 +43,7 @@ template <typename Adapter> class JobInterface {
4043
{ adapter.prepare_job_dispatch_impl(update_data) } -> std::same_as<void>;
4144
}
4245
{
43-
return static_cast<Adapter*>(this)->prepare_job_dispatch_impl(update_data);
46+
static_cast<Adapter*>(this)->prepare_job_dispatch_impl(update_data);
4447
}
4548

4649
template <typename UpdateDataset>
@@ -49,30 +52,15 @@ template <typename Adapter> class JobInterface {
4952
{ adapter.setup_impl(update_data, scenario_idx) } -> std::same_as<void>;
5053
}
5154
{
52-
return static_cast<Adapter*>(this)->setup_impl(update_data, scenario_idx);
55+
static_cast<Adapter*>(this)->setup_impl(update_data, scenario_idx);
5356
}
5457

5558
void winddown()
5659
requires requires(Adapter& adapter) { // NOSONAR
5760
{ adapter.winddown_impl() } -> std::same_as<void>;
5861
}
5962
{
60-
return static_cast<Adapter*>(this)->winddown_impl();
61-
}
62-
63-
void reset_logger()
64-
requires requires(Adapter& adapter) { // NOSONAR
65-
{ adapter.reset_logger_impl() } -> std::same_as<void>;
66-
}
67-
{
68-
static_cast<Adapter*>(this)->reset_logger_impl();
69-
}
70-
void set_logger(Logger& log)
71-
requires requires(Adapter& adapter) { // NOSONAR
72-
{ adapter.set_logger_impl(log) } -> std::same_as<void>;
73-
}
74-
{
75-
static_cast<Adapter*>(this)->set_logger_impl(log);
63+
static_cast<Adapter*>(this)->winddown_impl();
7664
}
7765

7866
private:

‎tests/cpp_unit_tests/test_job_dispatch.cpp‎

Lines changed: 7 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
#include <power_grid_model/batch_parameter.hpp>
66
#include <power_grid_model/common/common.hpp>
7+
#include <power_grid_model/common/dummy_logging.hpp>
78
#include <power_grid_model/common/exception.hpp>
89
#include <power_grid_model/common/multi_threaded_logging.hpp>
910
#include <power_grid_model/job_dispatch.hpp>
@@ -56,53 +57,37 @@ class JobAdapterMock : public JobInterface<JobAdapterMock> {
5657
JobAdapterMock& operator=(JobAdapterMock const& other) {
5758
if (this != &other) {
5859
counter_ = other.counter_;
59-
logger_ = nullptr; // reset logger
6060
}
6161
return *this;
6262
};
63-
JobAdapterMock(JobAdapterMock&& other) noexcept : logger_{other.logger_}, counter_{std::move(other.counter_)} {}
63+
JobAdapterMock(JobAdapterMock&& other) noexcept : counter_{std::exchange(other.counter_, nullptr)} {}
6464
JobAdapterMock& operator=(JobAdapterMock&& other) noexcept {
6565
if (this != &other) {
66-
counter_ = std::move(other.counter_);
67-
other.counter_ = nullptr;
68-
logger_ = other.logger_;
69-
other.logger_ = nullptr;
66+
counter_ = std::exchange(other.counter_, nullptr);
7067
}
7168
return *this;
7269
}
73-
~JobAdapterMock() { reset_logger(); };
70+
~JobAdapterMock() noexcept { counter_ = nullptr; }
7471

7572
void reset_counters() const { counter_->reset_counters(); }
7673
Idx get_calculate_counter() const { return counter_->calculate_calls; }
7774
Idx get_cache_calculate_counter() const { return counter_->cache_calculate_calls; }
7875
Idx get_setup_counter() const { return counter_->setup_calls; }
7976
Idx get_winddown_counter() const { return counter_->winddown_calls; }
80-
Idx get_set_logger_counter() const { return counter_->set_logger_calls; }
81-
Idx get_reset_logger_counter() const { return counter_->reset_logger_calls; }
8277

8378
private:
8479
friend class JobInterface<JobAdapterMock>;
8580

86-
Logger* logger_{};
8781
std::shared_ptr<CallCounter> counter_;
8882

89-
void calculate_impl(MockResultDataset const& /*result_data*/, Idx /*scenario_idx*/) const {
83+
void calculate_impl(MockResultDataset const& /*result_data*/, Idx /*scenario_idx*/,
84+
Logger const& /*logger*/) const {
9085
++(counter_->calculate_calls);
9186
}
92-
void cache_calculate_impl() const { ++(counter_->cache_calculate_calls); }
87+
void cache_calculate_impl(Logger const& /*logger*/) const { ++(counter_->cache_calculate_calls); }
9388
void prepare_job_dispatch_impl(MockUpdateDataset const& /*update_data*/) const { /* patch base class function */ }
9489
void setup_impl(MockUpdateDataset const& /*update_data*/, Idx /*scenario_idx*/) const { ++(counter_->setup_calls); }
9590
void winddown_impl() const { ++(counter_->winddown_calls); }
96-
void reset_logger_impl() {
97-
if (counter_ != nullptr) { // this may happen when the destructor is called on a moved object
98-
++(counter_->reset_logger_calls);
99-
}
100-
logger_ = nullptr;
101-
}
102-
void set_logger_impl(Logger& logger) {
103-
++(counter_->set_logger_calls);
104-
logger_ = &logger;
105-
}
10691
};
10792

10893
class SomeTestException : public std::runtime_error {
@@ -186,8 +171,6 @@ TEST_CASE("Test job dispatch logic") {
186171
CHECK(expected_result == actual_result);
187172
CHECK(adapter.get_calculate_counter() == 1);
188173
CHECK(adapter.get_cache_calculate_counter() == 0); // no cache calculation in this case
189-
CHECK(adapter.get_set_logger_counter() == 1);
190-
CHECK(adapter.get_reset_logger_counter() == 1);
191174
}
192175
SUBCASE("No scenarios") {
193176
bool const has_data = true;
@@ -200,8 +183,6 @@ TEST_CASE("Test job dispatch logic") {
200183
// no calculations should be done
201184
CHECK(adapter.get_calculate_counter() == 0);
202185
CHECK(adapter.get_cache_calculate_counter() == 0);
203-
CHECK(adapter.get_set_logger_counter() == 1);
204-
CHECK(adapter.get_reset_logger_counter() == 1);
205186
}
206187
SUBCASE("With scenarios and update data") {
207188
bool const has_data = true;
@@ -214,8 +195,6 @@ TEST_CASE("Test job dispatch logic") {
214195
// n_scenarios calculations should be done as we run sequentially
215196
CHECK(adapter.get_calculate_counter() == n_scenarios);
216197
CHECK(adapter.get_cache_calculate_counter() == 1); // cache calculation is done
217-
CHECK(adapter.get_set_logger_counter() == 2);
218-
CHECK(adapter.get_reset_logger_counter() == 2);
219198
}
220199
}
221200
SUBCASE("Test single_thread_job") {
@@ -241,8 +220,6 @@ TEST_CASE("Test job dispatch logic") {
241220
CHECK(adapter_.get_setup_counter() == expected_calls);
242221
CHECK(adapter_.get_winddown_counter() == expected_calls);
243222
CHECK(adapter_.get_calculate_counter() == expected_calls);
244-
CHECK(adapter_.get_set_logger_counter() == 1);
245-
CHECK(adapter_.get_reset_logger_counter() == 1);
246223
};
247224

248225
adapter.prepare_job_dispatch(update_data); // replicate preparation step from batch_calculation

0 commit comments

Comments
 (0)