Separate configuration from algorithms #200

BUYT-1 · 2023-04-03T11:11:33Z

Splits configuration-related things into a separate Configuration class, tests included.
TypoMiner's methods are reorganized in accordance with this class's structure.
Solves the problem with PliBasedFDAlgorithm's code repetition.
Better behavior in Python. More info about the options is provided to Python.

github-actions

clang-tidy made some suggestions

python_bindings/get_py_type.h

python_bindings/py_algorithm.h

python_bindings/py_ar_algorithm.h

python_bindings/py_to_any.h

src/algorithms/options/configuration.h

src/algorithms/options/option.h

github-actions

clang-tidy made some suggestions

src/algorithms/options/configuration.h

github-actions

clang-tidy made some suggestions

src/algorithms/options/configuration.h

github-actions

clang-tidy made some suggestions

src/algorithms/options/configuration.h

github-actions

clang-tidy made some suggestions

python_bindings/dataframe_reader.h

github-actions

clang-tidy made some suggestions

python_bindings/create_dataframe_reader.h

src/algorithms/cfd/cfd_discovery.cpp

src/algorithms/cfd/fd_first_algorithm.cpp

src/algorithms/typo_miner.cpp

src/algorithms/ucc/hyucc/hyucc.cpp

src/algorithms/ucc/ucc_algorithm.cpp

BUYT-1 · 2023-06-14T22:16:36Z

Проба пера. Получилось очень много бойлерплейта, но когда за макросом спрятал, стало терпимо.

С IOptionRegistry замысел в том, чтобы можно было вызвать шаблонную функцию, которая показывала бы, какие опции добавляет алгоритм. И от этого интерфейса отнаследовать другой класс специально для этих целей. Вторым параметром в эти статические методы должен передаваться nullptr по этому замыслу.

github-actions

clang-tidy made some suggestions

tests/test_configuration.cpp

github-actions

clang-tidy made some suggestions

src/algorithms/typo_miner.cpp

Only throw `std::invalid_argument`.

This avoids some unnecessary templates and emphasizes how the class is supposed to be used.

An algorithm's configuration is reset whenever `kwargs` is not empty. Also updated comments to explicitly state the supported Python usage scenarios.

BUYT-1 · 2023-07-12T05:37:46Z

Здесь как будто много кода по типу

auto Method(...) {
    return configuration.Method(...);
}

в алгоритме

polyntsov

Очень большой пр, логически отдельные части закидывай в будущем пожалуйста разными прами (форматирование pyro тут ни к чему, как минимум) :)
Не очень понимаю логику с передачей объектов опций как rvalue ref везде.
Хорошие тесты, кстати

src/core/parameters.h

polyntsov · 2023-07-22T19:26:45Z

src/util/config/option.h

+    Option &&SetValueCheck(ValueCheckFunc value_check) {
        assert(!value_check_);
        value_check_ = std::move(value_check);
-        return *this;
+        return std::move(*this);
    }


а зачем?

polyntsov · 2023-07-22T19:28:14Z

src/util/config/option.h

+    std::unique_ptr<IOption> MoveToHeap() override {
+        return std::make_unique<Option>(std::move(*this));
+    }


Не очень ясно зачем и как этот метод помогает. Как будто вызывающий код сам может сделать std::make_unique<Option>(opt), если нужном

polyntsov · 2023-07-22T19:33:57Z

src/util/config/configuration_stage.h

+    load_data = 0,
+    load_prepared_data,
+    execute


kLoadData, kLoadPreparedData, kExecute

Разве? Это же enum, а не кучка констант. В остальных enumах такие имена.

Ну enum это по сути и есть кучка констант с конкретными типом :)
В некоторых enumах имена такие только потому что мы хотим строки преобразовывать в значения енама и эти строки получаем от пользователя. Стайлгайд просит их именовать как константы

polyntsov · 2023-07-22T19:34:12Z

src/util/config/configuration_stage.h

+namespace util::config {
+
+// clang-format off
+BETTER_ENUM(ConfigurationStage, size_t,


а зачем size_t?

polyntsov · 2023-07-23T21:04:24Z

src/algorithms/typo_miner.cpp

+    // Options with the same names must have certain semantics. The
+    // situation where both algorithms have options with the same name but
+    // one throws an exception for the pair of values while the other does
+    // not is undefined behaviour. The situation where both algorithms may


Опять же, предлагаю не использовать "undefined behavior" в коде, кототрый мы контролируем :)

Так ведь оно сейчас так же работает. Просто здесь это явно написано. "Программистская ошибка", может быть, точнее будет

Так ведь оно сейчас так же работает.

Не очень понял, что так же работает (и как, так же?).
Я просто предлагаю избегать ситуаций в коде, которые нарушают какие-либо инварианты и это никак не отлавливается, а вместо этого говорится, что это "undefined behavior" и разбирайтесь сами. Нам же потом это и дебажить ведь. Стоит как минимум писать ассерты, проверяющие нужные инварианты. Тут такой ассерт уже есть насколько я вижу, поэтому просто предлагаю убрать "undefined behavior", а написать, что это просто ожидания этого кода.

polyntsov · 2023-07-23T21:15:15Z

src/algorithms/create_algorithm.h

@@ -28,16 +24,21 @@ std::unique_ptr<AlgorithmBase> CreateAlgorithmInstance(AlgorithmType algorithm)
            static_cast<size_t>(algorithm), create);
 }

-template <typename AlgorithmBase>
-std::vector<AlgorithmType> GetAllDerived() {
+template <typename DerivedFrom>


по-моему Base лучше, чем DerivedFrom, такой нейминг путает

polyntsov · 2023-07-23T21:21:21Z

src/algorithms/typo_miner.cpp

+        if (option_name == kPliRelation) {
+            throw std::logic_error("You do not manage this option.");
+        }


довольно плохо выглядит, но решения лучше я придумать не могу

polyntsov · 2023-07-23T21:24:53Z

tests/test_configuration.cpp

+
+TEST_F(ConfigurationTesting, BasicConfiguration) {
+    int var1;
+    const int var1val = 4;


nit: constexpr

polyntsov · 2023-07-23T21:25:50Z

tests/test_configuration.cpp

+using FuncTuple = Configuration::FuncTuple;
+using OptionNameSet = Configuration::OptionNameSet;
+using AddExternalOptFunc = Configuration::AddExternalOptFunc;
+using SetExternalOptFunc = Configuration::SetExternalOptFunc;
+using UnsetExternalOptFunc = Configuration::UnsetExternalOptFunc;
+using GetExternalTypeIndexFunc = Configuration::GetExternalTypeIndexFunc;
+using ResetExternalAlgoConfigFunc = Configuration::ResetExternalAlgoConfigFunc;
+
+class ConfigurationTesting : public ::testing::Test {


nit: anonymous namespace

github-actions bot reviewed Apr 3, 2023

View reviewed changes

BUYT-1 force-pushed the separate-config branch 3 times, most recently from 53dae04 to b21c6a5 Compare April 6, 2023 08:34

github-actions bot reviewed Apr 6, 2023

View reviewed changes

src/algorithms/options/configuration.h Outdated Show resolved Hide resolved

BUYT-1 force-pushed the separate-config branch 2 times, most recently from ec37a4b to 2e29f49 Compare April 6, 2023 08:45

BUYT-1 mentioned this pull request Apr 6, 2023

Simplify configuration #196

Merged

github-actions bot reviewed Apr 6, 2023

View reviewed changes

src/algorithms/options/configuration.h Outdated Show resolved Hide resolved

github-actions bot reviewed Apr 6, 2023

View reviewed changes

src/algorithms/options/configuration.h Outdated Show resolved Hide resolved

BUYT-1 force-pushed the separate-config branch 4 times, most recently from 62b33e1 to 030e869 Compare April 10, 2023 08:18

BUYT-1 force-pushed the separate-config branch 5 times, most recently from edf3df4 to 6de65f6 Compare April 20, 2023 07:37

BUYT-1 force-pushed the separate-config branch 2 times, most recently from 08b8905 to 1237dd0 Compare April 29, 2023 10:34

github-actions bot reviewed Apr 29, 2023

View reviewed changes

python_bindings/dataframe_reader.h Outdated Show resolved Hide resolved

BUYT-1 force-pushed the separate-config branch from 1237dd0 to 469f35f Compare April 29, 2023 19:01

BUYT-1 mentioned this pull request May 19, 2023

Miscellaneous config fixups #214

Merged

BUYT-1 force-pushed the separate-config branch 2 times, most recently from bdc91f4 to 20c5069 Compare June 14, 2023 21:21

github-actions bot reviewed Jun 14, 2023

View reviewed changes

BUYT-1 force-pushed the separate-config branch from 20c5069 to 78e7147 Compare June 14, 2023 22:02

BUYT-1 force-pushed the separate-config branch from 78e7147 to 0279169 Compare June 16, 2023 11:39

github-actions bot reviewed Jul 8, 2023

View reviewed changes

BUYT-1 force-pushed the separate-config branch 5 times, most recently from 827bc8f to 656f217 Compare July 10, 2023 15:00

github-actions bot reviewed Jul 10, 2023

View reviewed changes

src/algorithms/typo_miner.cpp Show resolved Hide resolved

BUYT-1 force-pushed the separate-config branch 2 times, most recently from 8d10b21 to 2c4bd78 Compare July 10, 2023 15:35

BUYT-1 marked this pull request as ready for review July 10, 2023 15:38

BUYT-1 force-pushed the separate-config branch from 2c4bd78 to cdbc8f0 Compare July 10, 2023 16:15

BUYT-1 added 14 commits July 12, 2023 01:25

Add static_assert for algorithm tuple size

769c892

Unify option value checks exceptions

8c99ae8

Only throw `std::invalid_argument`.

Add GetDescription to IOption

5774e0f

Rename Pyro's Configuration to Parameters

ee9ee8b

Fix formatting in Pyro-related files

65791dc

Add MoveToHeap method to Option

441b783

This avoids some unnecessary templates and emphasizes how the class is supposed to be used.

Add ConfigurationStage enum

52ba872

Add Configuration

d1fdf4b

Use Configuration in algorithms

ea3b4c2

Move IsDerived check to a separate function

6a61d4c

Set PliBasedAlgorithm::relation_ using Configuration

8284f45

Reset algorithm configuration when used in Python console

8709c53

An algorithm's configuration is reset whenever `kwargs` is not empty. Also updated comments to explicitly state the supported Python usage scenarios.

Add tests for Configuration

1a62c95

Get descriptions and possible options in Python

978f63d

BUYT-1 force-pushed the separate-config branch from cdbc8f0 to 978f63d Compare July 11, 2023 18:25

polyntsov requested changes Jul 23, 2023

View reviewed changes

BUYT-1 marked this pull request as draft July 29, 2023 16:28

BUYT-1 mentioned this pull request Aug 15, 2023

Add GetPossibleOptions to Algorithm #248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate configuration from algorithms #200

Separate configuration from algorithms #200

BUYT-1 commented Apr 3, 2023 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

BUYT-1 commented Jun 14, 2023

github-actions bot left a comment

github-actions bot left a comment

BUYT-1 commented Jul 12, 2023

polyntsov left a comment

polyntsov Jul 22, 2023

polyntsov Jul 22, 2023

polyntsov Jul 22, 2023

BUYT-1 Jul 25, 2023

polyntsov Jul 25, 2023

polyntsov Jul 22, 2023

polyntsov Jul 23, 2023

BUYT-1 Aug 18, 2023

polyntsov Aug 18, 2023

polyntsov Jul 23, 2023

polyntsov Jul 23, 2023

polyntsov Jul 23, 2023

polyntsov Jul 23, 2023

Separate configuration from algorithms #200

Are you sure you want to change the base?

Separate configuration from algorithms #200

Conversation

BUYT-1 commented Apr 3, 2023 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

BUYT-1 commented Jun 14, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

BUYT-1 commented Jul 12, 2023

polyntsov left a 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

Choose a reason for hiding this comment

BUYT-1 commented Apr 3, 2023 •

edited

Loading