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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Joshy-R
Copy link
Contributor

@Joshy-R Joshy-R commented Mar 9, 2025

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:

  • The pull request only contains commits that are related to it.
  • 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.

Copy link

codecov bot commented Mar 9, 2025

Codecov Report

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

Project coverage is 92.3%. Comparing base (022fc61) to head (a338162).

Files with missing lines Patch % Lines
include/mqt-core/dd/MemoryManager.hpp 84.6% 6 Missing ⚠️
include/mqt-core/dd/FunctionalityConstruction.hpp 95.7% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #856     +/-   ##
=======================================
- Coverage   92.3%   92.3%   -0.1%     
=======================================
  Files        137     136      -1     
  Lines      13619   13618      -1     
  Branches    2104    2104             
=======================================
- Hits       12575   12570      -5     
- Misses      1044    1048      +4     
Flag Coverage Δ
cpp 92.0% <94.2%> (-0.1%) ⬇️
python 99.7% <ø> (ø)
Files with missing lines Coverage Δ
include/mqt-core/dd/UniqueTable.hpp 97.1% <ø> (ø)
...mqt-core/dd/statistics/MemoryManagerStatistics.hpp 100.0% <100.0%> (ø)
include/mqt-core/dd/FunctionalityConstruction.hpp 95.7% <95.7%> (ø)
include/mqt-core/dd/MemoryManager.hpp 88.0% <84.6%> (-12.0%) ⬇️
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Joshy-R Joshy-R force-pushed the move_templated_function_and_methods_to_header_files_for_reuse_in_dd_sim branch 2 times, most recently from 45cc0d7 to cf80f8a Compare March 9, 2025 20:06
@Joshy-R Joshy-R force-pushed the move_templated_function_and_methods_to_header_files_for_reuse_in_dd_sim branch from cf80f8a to a338162 Compare March 9, 2025 20:12
@Joshy-R
Copy link
Contributor Author

Joshy-R commented Mar 9, 2025

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

@burgholzer
Copy link
Member

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 burgholzer added refactor Anything related to code refactoring DD Anything related to the DD package c++ Anything related to C++ code labels Mar 9, 2025
@burgholzer burgholzer added this to the DD Package Improvements milestone Mar 9, 2025
@Joshy-R
Copy link
Contributor Author

Joshy-R commented Mar 10, 2025

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.

@Joshy-R
Copy link
Contributor Author

Joshy-R commented Mar 10, 2025

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?

@burgholzer
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Anything related to C++ code DD Anything related to the DD package refactor Anything related to code refactoring
Projects
Status: In Progress
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants