Closed lsschmid closed 5 months ago
codecov[bot]
commented
6 months ago Attention: Patch coverage is 94.11765% with 6 lines in your changes missing coverage. Please review.
Project coverage is 91.5%. Comparing base (
42005a9) to head (8c2f230). Report is 106 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| include/mqt-core/operations/AodOperation.hpp | 91.6% | 3 Missing :warning: |
| src/operations/StandardOperation.cpp | 0.0% | 3 Missing :warning: |
lsschmid
commented
6 months ago Everything except the linter passes.
I checked that all linter warnings that still remain are not from my code.
lsschmid
commented
5 months ago Finally found the time to go through this PR 😌 Overall the changes here look good 👍 I pushed two minor changes directly and added further smaller comments in the files themselves. Most of these should be pretty minor.
I have one medium-large question and one bigger question I'd like to get out of the way before proceeding here:
First, what about reading the produced QASM circuits back in? Right now, no adaptation on the QASM parser has been performed so far. So the produced QASM files cannot be read anywhere. While not a dealbreaker, it would certainly be nice to support that, wouldn't it?
Second, and probably the bigger question: @ystade recently introduced the
nasubcomponent in mqt-core and mqt-qmap, where he (amongst other things) also defined a ShuttlingOperation (with load, move, and store types). Now this PR implements essentially the same concept, but in a different fashion. While I would be ok with that in the short-term, this duplication is certainly no maintainable solution for the future. How hard do you think it would be to base your changes (especially those in QMAP) on @ystade's infrastructure for neutral atom computations? From the changes here, the QASM dump would be the most important thing to port to the existingNAComputationandNAOperationclasses. (Expect me asking a similar question in the mqt-qmap PR)
Regarding the qasm import: Back when I wrote the code I also added the correct parsing commands. This took me quite some time as I'm not really familiar with parsing in general. Unfortuantly in the meantime the parser was updated to support qasm3 which broke my version and I could not see an easy way to transfer my code. As for now, the only usecase where the parsing would be interesting is to schedule circuits which have been mapped and saved previously. This is indeed a possible/interesting option but currently not used by us and my knowledge on parsing is not sufficient to do some "quick fix" to that.
Regarding the NA unification: Obviously, my code already existed before @ystade started with his part. We had some discussion back then on reusing the code but then decided to create two separate versions as a start as we required different things. A unification might probably be possible and would probably make sense for common export/import but currently this is a non-trivial task as we took different approaches to how we handle/represent the AOD moves.
burgholzer
commented
5 months ago Finally found the time to go through this PR 😌 Overall the changes here look good 👍 I pushed two minor changes directly and added further smaller comments in the files themselves. Most of these should be pretty minor. I have one medium-large question and one bigger question I'd like to get out of the way before proceeding here: First, what about reading the produced QASM circuits back in? Right now, no adaptation on the QASM parser has been performed so far. So the produced QASM files cannot be read anywhere. While not a dealbreaker, it would certainly be nice to support that, wouldn't it? Second, and probably the bigger question: @ystade recently introduced the
nasubcomponent in mqt-core and mqt-qmap, where he (amongst other things) also defined a ShuttlingOperation (with load, move, and store types). Now this PR implements essentially the same concept, but in a different fashion. While I would be ok with that in the short-term, this duplication is certainly no maintainable solution for the future. How hard do you think it would be to base your changes (especially those in QMAP) on @ystade's infrastructure for neutral atom computations? From the changes here, the QASM dump would be the most important thing to port to the existingNAComputationandNAOperationclasses. (Expect me asking a similar question in the mqt-qmap PR)Regarding the qasm import: Back when I wrote the code I also added the correct parsing commands. This took me quite some time as I'm not really familiar with parsing in general. Unfortuantly in the meantime the parser was updated to support qasm3 which broke my version and I could not see an easy way to transfer my code. As for now, the only usecase where the parsing would be interesting is to schedule circuits which have been mapped and saved previously. This is indeed a possible/interesting option but currently not used by us and my knowledge on parsing is not sufficient to do some "quick fix" to that.
Fully understand that point. The new parser is structured quite a bit differently than the old one. Adding a parser for the new operations would definitely make sense in the future. Also triggered by the comment below, I would postpone that implementation to a point where we consolidated on the particular details when it comes to NA Compilation and how to represent the corresponding computations. At that point, this can be a student project. Might be worth tracking that in an issue.
It isn't valid QASM in either version of the standard.
For it to be valid, you would have to add an opaque gate declaration for the operation to the top of the file, eliminate the ; from the parameter list, and you'd have to somehow handle the fact that the parameter list can have arbitrary length.
Regarding the NA unification: Obviously, my code already existed before @ystade started with his part. We had some discussion back then on reusing the code but then decided to create two separate versions as a start as we required different things. A unification might probably be possible and would probably make sense for common export/import but currently this is a non-trivial task as we took different approaches to how we handle and represent the AOD moves.
It's obvious that this is a non-trivial task. It's still important to think about this early as I would really want to avoid to develop two completely separate strands of features here.
Just for clarification:
To me, the two AOD representations do not look fundamentally different. You choose to explicitly encode the direction and use floating point numbers for start and end coordinates.
@ystade chose to implicitly store the direction by assuming a 2D plane and using (x, y) (integer) coordinates for the start and end coordinates. Functionality-wise, these two are equivalent (at least in 2D and assuming the integer resolution is enough).
Do you really think that adapting your code to that class would be that much of an effort? Most likely, I am simply overlooking things given that I have not spent enough time with the QMAP code yet. So I am really interested in your judgement here.
If this is too much to ask for now (which is totally ok), then I'd at least like to have a clear plan on who has the responsibility to consolidate that in the future and a rough timeframe.
lsschmid
commented
5 months ago Fully understand that point. The new parser is structured quite a bit differently than the old one. Adding a parser for the new operations would definitely make sense in the future. Also triggered by the comment below, I would postpone that implementation to a point where we consolidated on the particular details when it comes to NA Compilation and how to represent the corresponding computations. At that point, this can be a student project. Might be worth tracking that in an issue. It isn't valid QASM in either version of the standard. For it to be valid, you would have to add an
opaquegate declaration for the operation to the top of the file, eliminate the;from the parameter list, and you'd have to somehow handle the fact that the parameter list can have arbitrary length.Yes, exactly; this would probably be the way to go, also for the consolidated version below.
It's obvious that this is a non-trivial task. It's still important to think about this early as I would really want to avoid to develop two completely separate strands of features here. Just for clarification: To me, the two AOD representations do not look fundamentally different. You choose to explicitly encode the direction and use floating point numbers for start and end coordinates. @ystade chose to implicitly store the direction by assuming a 2D plane and using
(x, y)(integer) coordinates for the start and end coordinates. Functionality-wise, these two are equivalent (at least in 2D and assuming the integer resolution is enough). Do you really think that adapting your code to that class would be that much of an effort? Most likely, I am simply overlooking things given that I have not spent enough time with the QMAP code yet. So I am really interested in your judgement here. If this is too much to ask for now (which is totally ok), then I'd at least like to have a clear plan on who has the responsibility to consolidate that in the future and a rough timeframe.That's correct. As the basic functionality is quite clear (moving coordinates in 2D), I think both sides could be consolidated in a straightforward fashion conceptional-wise, but with small differences (e.g., I require floats instead of int) . An almost similar problem exists for the NeutralAtomArchitecture Class we used for the Mapping Tasks. For now I would keep the two projects separate and then discuss on an overarching Operation Class + Architecure Class in the following weeks, also in the context of a compiler collection.
lsschmid
commented
5 months ago I'll write the issues 👍
Add Hybrid NA Mapper Functionality
This adds two things:
Checklist: