Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Add macro to simplify algorithm binding #1510

Merged

Conversation

benjaminhuth
Copy link
Member

While working with the python bindings, I thought its quite a lot of repetive boiler plate code to add an Algorithm to the python bindings. With the help of Boost.Preprocessor this can be simplified quite a lot:

ACTS_PYTHON_DECLARE_ALGORITHM(
    ActsExamples::FooAlgorithm,
    "FooAlgorithm", 
    inputA, inputB, output)

expands to

{
  using Alg = ActsExamples::FooAlgorithm;
  using Config = Alg::Config;
  auto alg = py::class_<Alg, ActsExamples::BareAlgorithm, std::shared_ptr<Alg>>(
                 mex, "FooAlgorithm")
                 .def(py::init<const Config &, Acts::Logging::Level>(),
                      py::arg("config"), py::arg("level"))
                 .def_property_readonly("config", &Alg::config);
  auto c = py::class_<Config>(alg, "Config").def(py::init<>());
  ACTS_PYTHON_STRUCT_BEGIN(c, Config);
  ACTS_PYTHON_MEMBER(inputA);
  ACTS_PYTHON_MEMBER(inputB);
  ACTS_PYTHON_MEMBER(output);
  ACTS_PYTHON_STRUCT_END();
}

This supports any number of arguments due to the preprocessor loops of Boost. Since we use macros anyways here to simplify declaration, I think this could be an acceptable use case of another macro.

I have this already appied for the Exa.TrkX algorithm, but wanted to wait for feedback (@paulgessinger) before doing the rest of the algorithms.

@benjaminhuth benjaminhuth added Component - Examples Affects the Examples module 🚧 WIP Work-in-progress labels Sep 12, 2022
@benjaminhuth benjaminhuth added this to the WIP milestone Sep 12, 2022
@codecov
Copy link

codecov bot commented Sep 12, 2022

Codecov Report

Merging #1510 (4de4b56) into main (eef2627) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1510   +/-   ##
=======================================
  Coverage   48.54%   48.54%           
=======================================
  Files         381      381           
  Lines       20789    20789           
  Branches     9539     9539           
=======================================
  Hits        10092    10092           
  Misses       4112     4112           
  Partials     6585     6585           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@paulgessinger paulgessinger changed the title refactor: Add marco to simplify algorithm binding refactor: Add macro to simplify algorithm binding Sep 13, 2022
@benjaminhuth benjaminhuth removed the 🚧 WIP Work-in-progress label Sep 13, 2022
@paulgessinger
Copy link
Member

Existing python tests seem to fail with this change. Can you check if you can reproduce locally, @benjaminhuth?

@benjaminhuth
Copy link
Member Author

Yeah I already know what the problem is. Most writers/readers do not provide a config() accessor in C++, so I left the corresponding macro without defining that in python.

However, it seems like that the config property it is used in one place at least.

I see three options there

  • remove that one use case
  • Add config accessors to all readers and writers and defining that in the macro
  • Don't use the macro in the one special case (not a good solution I think)

I would go with the second option, because I think it is useful to have this accessor... What do you think @paulgessinger ?

@paulgessinger
Copy link
Member

Yes I agree, adding the config accessor everywhere seems like the right thing to do.

@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label Sep 19, 2022
@paulgessinger paulgessinger modified the milestones: WIP, next Oct 3, 2022
@benjaminhuth benjaminhuth removed the 🚧 WIP Work-in-progress label Oct 13, 2022
@benjaminhuth
Copy link
Member Author

@paulgessinger This should be ready to go in now, could you have a look and approve?

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Looks good I think. If the output doesn't change, I'm happy.

Examples/Python/src/Json.cpp Outdated Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit eaa9d5f into acts-project:main Oct 18, 2022
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Oct 20, 2022
While working with the python bindings, I thought its quite a lot of repetive boiler plate code to add an Algorithm to the python bindings. With the help of `Boost.Preprocessor` this can be simplified quite a lot:

```c++ 
ACTS_PYTHON_DECLARE_ALGORITHM(
    ActsExamples::FooAlgorithm,
    "FooAlgorithm", 
    inputA, inputB, output)
```

expands to 

```c++
{
  using Alg = ActsExamples::FooAlgorithm;
  using Config = Alg::Config;
  auto alg = py::class_<Alg, ActsExamples::BareAlgorithm, std::shared_ptr<Alg>>(
                 mex, "FooAlgorithm")
                 .def(py::init<const Config &, Acts::Logging::Level>(),
                      py::arg("config"), py::arg("level"))
                 .def_property_readonly("config", &Alg::config);
  auto c = py::class_<Config>(alg, "Config").def(py::init<>());
  ACTS_PYTHON_STRUCT_BEGIN(c, Config);
  ACTS_PYTHON_MEMBER(inputA);
  ACTS_PYTHON_MEMBER(inputB);
  ACTS_PYTHON_MEMBER(output);
  ACTS_PYTHON_STRUCT_END();
}
```

This supports any number of arguments due to the preprocessor loops of Boost. Since we use macros anyways here to simplify declaration, I think this could be an acceptable use case of another macro.

I have this already appied for the Exa.TrkX algorithm, but wanted to wait for feedback (@paulgessinger) before doing the rest of the algorithms.
paulgessinger pushed a commit that referenced this pull request Oct 20, 2022
While working with the python bindings, I thought its quite a lot of repetive boiler plate code to add an Algorithm to the python bindings. With the help of `Boost.Preprocessor` this can be simplified quite a lot:

```c++ 
ACTS_PYTHON_DECLARE_ALGORITHM(
    ActsExamples::FooAlgorithm,
    "FooAlgorithm", 
    inputA, inputB, output)
```

expands to 

```c++
{
  using Alg = ActsExamples::FooAlgorithm;
  using Config = Alg::Config;
  auto alg = py::class_<Alg, ActsExamples::BareAlgorithm, std::shared_ptr<Alg>>(
                 mex, "FooAlgorithm")
                 .def(py::init<const Config &, Acts::Logging::Level>(),
                      py::arg("config"), py::arg("level"))
                 .def_property_readonly("config", &Alg::config);
  auto c = py::class_<Config>(alg, "Config").def(py::init<>());
  ACTS_PYTHON_STRUCT_BEGIN(c, Config);
  ACTS_PYTHON_MEMBER(inputA);
  ACTS_PYTHON_MEMBER(inputB);
  ACTS_PYTHON_MEMBER(output);
  ACTS_PYTHON_STRUCT_END();
}
```

This supports any number of arguments due to the preprocessor loops of Boost. Since we use macros anyways here to simplify declaration, I think this could be an acceptable use case of another macro.

I have this already appied for the Exa.TrkX algorithm, but wanted to wait for feedback (@paulgessinger) before doing the rest of the algorithms.
@benjaminhuth benjaminhuth deleted the brefactor/python-algorithm-macro branch October 24, 2022 08:14
@paulgessinger paulgessinger modified the milestones: next, v21.0.0 Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants