mqt-core icon indicating copy to clipboard operation
mqt-core copied to clipboard

♻️ Move templated function and methods to header files for reuse in dd sim

Open rotmanjanez opened this issue 1 year ago • 7 comments

Description

Moved several templated function definitions with explicit instantiation in source files to the header files. This is the first step for moving the deterministic noise simulation from core to mqt-ddsim, allowing the other repo to reuse much of the code when moving dNode and dEdge.

Checklist:

  • [x] The pull request only contains commits that are related to it.
  • [x] I have added appropriate tests and documentation.
  • [ ] I have made sure that all CI jobs on GitHub pass.
  • [ ] The pull request introduces no new warnings and follows the project's style guidelines.

rotmanjanez avatar Mar 09 '25 19:03 rotmanjanez

Codecov Report

:x: Patch coverage is 94.23077% with 9 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
include/mqt-core/dd/MemoryManager.hpp 84.6% 6 Missing :warning:
include/mqt-core/dd/FunctionalityConstruction.hpp 95.7% 3 Missing :warning:

:loudspeaker: Thoughts on this report? Let us know!

codecov[bot] avatar Mar 09 '25 19:03 codecov[bot]

the linter errors, that #include <stack> is not directly used, but removing it make the build fail. Any ideas?

rotmanjanez avatar Mar 09 '25 20:03 rotmanjanez

the linter errors, that #include <stack> is not directly used, but removing it make the build fail. Any ideas?

Hm. That's a strange one. But I suppose this is somewhat due to the functions being templated so they are essentially inlined in the compilation units where they are needed. Clang-tidy does not really perform that well with templates like these. For this particular error, the proper "fix" would probably be to inlcude a

// NONLINTNEXTLINE(misc-include-cleaner)

right before the probalematic import.

I believe you could technically make the code compile by removing the include in the header file and adding it to the source file where the buildFunctionality function is used (probably in the tests). Although that is a truly ugly solution for a problem that should not be one.


I have a more general idea though. I'd actually rather prefer to keep the definitions of these functions in the source files with the explicit template instantiations because that really helps with compile times. You can, instead, add the necessary additional explicit template instantiation directly in DDSim and that "just works". They need to be in the translation unit where the respective class is first used. Given how it will just take the template instantiations for the density matrix related code, that should not be too bad.

If I am not mistaken that would reduce this PR to removing that one static_assert, which I definitely needs to go.

In the long-run, I truly hope that we can completely eliminate the templates and just switch to a runtime config for the various compute and unique tables. That needs proper benchmarking though, as it also means that we have to switch from std::array to std::vector in quite some places.

Does that make sense?

burgholzer avatar Mar 09 '25 22:03 burgholzer

You can, instead, add the necessary additional explicit template instantiation directly in DDSim and that "just works".

As far as i know, explicit instantiation only work in the same file. Also tried it with a small example and count not get it to compile with template definitions in source files and explicit instantiation in another file. Am i missing something?

I think compile times are not that bad atm, It probably could be improved if containers (e.g. UniqueTable) are type agnostic.

rotmanjanez avatar Mar 10 '25 10:03 rotmanjanez

I can fist start working on making the MemoryManager and UniqueTable less templated to improve compile times. Then, only FunctionalityConstruction would be affected by more code in header files. Would this be an acceptable solution?

rotmanjanez avatar Mar 10 '25 10:03 rotmanjanez

You can, instead, add the necessary additional explicit template instantiation directly in DDSim and that "just works".

As far as i know, explicit instantiation only work in the same file. Also tried it with a small example and count not get it to compile with template definitions in source files and explicit instantiation in another file. Am i missing something?

Hm. I remember getting that to work somehow. But maybe I am just misremembering.

I can fist start working on making the MemoryManager and UniqueTable less templated to improve compile times. Then, only FunctionalityConstruction would be affected by more code in header files. Would this be an acceptable solution?

That sounds very reasonable. I think one can take this a step further: Anything in buildFunctionality is only relevant for Matrix DDs and not for Density Matrix DDs. So it might be that the respective code can just stay in the source file as it is right now. These functions should neither be relevant for the deterministic noise-aware simulator nor for the stochastic one.

burgholzer avatar Mar 10 '25 11:03 burgholzer

I assume this PR here can be closed for now, right @rotmanjanez?

burgholzer avatar Mar 30 '25 18:03 burgholzer