Create dedicated class for dds #887
Conversation
There was a problem hiding this comment.
Hey @rotmanjanez 👋🏼
Thanks for the PR again!
Sorry that it took a while to get to it. This week has been a bit crazy.
I definitely see where you are going with this PR.
I left some inline comments on some changes that can definitely go in, independent of whether we actually want to go into the direction of this PR.
I see some of the value that comes from this PR. However, I feel it is hard to execute this idea fully.
There is still quite some code in the Package.hpp file that either concerns, vectors, matrices or a mixture of both. And due to the mixture and the use of templates, these will be hard to pull apart.
While this then helps to reduce the size of the Package.hpp/cpp files, it also splits up related code into unrelated files.
Where I see the most value at the moment:
Moving the state preparation and gate matrix preparation code somewhere isolated. I have already spent quite some time in the past to make the Edges.hpp file contain a little more code that was previously located in the Package.hpp file.
Maybe it would make sense to have dedicated VectorDD, MatrixDD, and DensityMatrixDD classes that define the respective functions. One could put these classes into separate files in a common subfolder. (maybe that's too much already though).
Instead of being members of the Package class, these functions would take a Package instance as an argument. Or even just the necessary data structures like the MemoryManager or the UniqueTable (whatever is needed).
Does that make any sense? What do you think?
I believe the general direction of trying to make the Package class smaller and isolating certain aspects better is worthwhile pursuing.
There was a problem hiding this comment.
The changes here can go in regardless of whether we actually pursue the direction of this PR. 😌
There was a problem hiding this comment.
The changes here can go in regardless of whether we actually pursue the direction of this PR. 😌
There was a problem hiding this comment.
The changes here can go in regardless of whether we actually pursue the direction of this PR. 😌
| static constexpr std::size_t DEFAULT_NUM_BUCKETS = NBUCKET; | ||
|
|
There was a problem hiding this comment.
That must have been an oversight in my previous PR. The NBUCKET template argument no longer makes sense here.
Would you mind removing it and just setting
static constexpr std::size_t DEFAULT_NUM_BUCKETS = 32768U;
An then in the constructor below
explicit UnaryComputeTable(const size_t numBuckets = DEFAULT_NUM_BUCKETS) {
|
|
||
| #pragma once | ||
|
|
||
| #include "dd/DesicionDiagramContainer.hpp" |
There was a problem hiding this comment.
I cannot find this file anywhere. Could it be that you forgot to commit it?
There was a problem hiding this comment.
Maybe I have overlooked it, but I would like to keep these aliases.
They were previously under the qc:: namespace, which didn't actually make too much sense.
"Edge" is just an awkward name for referring to a whole DD. Eventually, I'd like to phase that out somehow.
But for now, I'd like to keep the aliases.
There was a problem hiding this comment.
Depending on the outcome of a PR that I plan to open up soon, this might eventually no longer be needed. Given the imminent release of mqt-core v3, now would be a perfect time to switch to C++20. I'll have to see how much of the existing ecosystem breaks upon switching.
Description
This is a draft for grouping resource management for DDs in a class. There, the MemoryManager, a UniqueTable and Caches for operations within the same DD-Type are grouped. This has the benefit that it is easier to see in which contexts Caches etc, are used and makes some logic with garbage collection a bit easier. This also splits up the large Package.cpp into two smaller files — One for vector sand one for matrices.
This however has the downside that some operations need to be either forwarded (not done yet) or a dedicated call to the subobject is necessary (now
matrices(),vectors()ordensities()). I first thought this would be a nice refractor, but I'm not sure if I'm convinced this is a good solution. What do you think about this @burgholzer?Checklist: