search

QuTech-Delft / OpenQL

OpenQL: A Portable Quantum Programming Framework for Quantum Accelerators. https://dl.acm.org/doi/10.1145/3474222
https://openql.readthedocs.io
Other
99 stars 44 forks source link

Finish modularization, and lots of refactoring #405

Closed jvanstraten closed 3 years ago

jvanstraten commented 3 years ago

Followup on #364

In this PR

Future refactoring/utility work (i.e. postponed for now)

jvanstraten commented 3 years ago

Use case 1: the old-fashioned way

The result must be the same as before the refactoring/pass management rewrite, for as far as passes/options were not removed.

This is tested by every test in the test suite from before pass management was updated.

Use case 2: no changes to Python, only to config files

Basically, the old-fashioned way within python, but with compiler configuration specified in the platform, with compatibility mode enabled.

TODO: test case(s).

Use case 3: using a mix of global and pass options within Python

That is, use the new system for new features, but with as much backward compatibility as possible.

Use case 4: using pass management options only

Use case 5: like 4, but with the new flow as designed for modularity initially

jvansomeren commented 3 years ago

Clean build. Clean docbuild. Now reading documentation.

Python API: get_passtypes retrieves documentation but name looks like something retrieving a handle on e.g. the passes: "get...". Prefer that documentation retrieval gets other name prefixes such as "print", as used by later APIs.

jvanstraten commented 3 years ago

Python API: get_passtypes retrieves documentation but name looks like something retrieving a handle on e.g. the passes: "get...". Prefer that documentation retrieval gets other name prefixes such as "print", as used by later APIs.

@jvansomeren

While print_* versions of all documentation retrieval functions exist, just dumping to stdout is not always what you want. I would argue that the primitive function is always a getter that returns a string (which you can then print, or in the case of docs/conf.py convert to reStructuredText fragments), but for compatibility and consistency a print_* version should exist for all documentation getters as well.

That said, the name of that particular doc getter could indeed be changed to better express what it does, for example [get|print]_pass_type_docs(), otherwise it might be confused with something that just returns a list of available pass type names (for which there is no API right now, though one could easily be added). Alternatively I could replace the get_ prefix with for example dump_ for all documentation getters. Thoughts?

ETA: to be clear, the difference between the get_ and print_ functions is that the former returns the result as a string, while the latter prints it to stdout and returns None/void. Otherwise they are identical (both API calls chain to dump_* in the C++ world, which takes a std::ostream and line prefix string as arguments).

jvanstraten commented 3 years ago

@wvlothuizen

Wheels built by CI according to e426c1e (Python 3.9) and 933163e (Python 3.6 through 3.8) should now be available for further testing via pip3 install qutechopenql==0.8.1.dev6 for Windows, MacOS, and Linux (but of course the new APIs are still unstable as per code review).

jvansomeren commented 3 years ago

I would then propose to prefix the string returning getters with dump instead of get.

jvansomeren commented 3 years ago

In Python API, compilation strategy is mentioned, a default compilation strategy which not be good enough and that you can change it. Only in class Platform it is described (with the compiler config) while it should be described in class Compiler at least.

jvansomeren commented 3 years ago

What is the use of pass groups? One can make them, set options to subpasses. There are so many interfaces but it is not clear when reading them why they are there. The compiler config file allows to specify a hierarchy. Wildcards are allowed in set_option but only (?) to select subpasses, seemingly not as prefix of the path. Set_option_recursively sets an option to all passes (that support the option). I propose to undocument pass groups or to take them out of the implementation until we have a clear use case for them.

jvanstraten commented 3 years ago

Some use cases I envisioned for pass groups, off the top of my head:

Before we decided to remove the many CC-light-specific passes, I was intending to group the passes that were previously hardcoded as a group in backend in the default configuration. I intended to do the same with the mapper (make subpasses for initial placement, heuristic routing, and the post-route decomposition thing), but tearing those components apart was more difficult than I thought, so I ended up giving up on it. So, right now, indeed, I don't see a practical use case.

Even still, I don't understand why you'd then remove a feature that's already been built and documented. Sure, if something breaks or is otherwise in the way and no one is using it, get rid of it, I guess. But this hasn't even seen the light of day yet.

In general I find the "I don't see a use case" argument moot, because someone else might. You even found yourself at the other end of the argument with QI and the mapper. Why not present the user with an option to enable it (if indeed it is simple to implement)? Because the others "didn't see a use case." It feels so counterproductive. Isn't the end goal to make a tool, something that's useful in general? Not for ourselves, but for our users, so if there's any scenario where something might be helpful to someone now or in the future, why not leave it in? An elegant interface is one where everything has been reduced to the basic building blocks (perhaps with shorthand notation for commonly-used things), but with as many of those building blocks as possible.

If you feel strongly about this though, well, deleting stuff is easy...

Wildcards are allowed in set_option but only (?) to select subpasses, seemingly not as prefix of the path

I'm not sure what you mean by this exactly. But the notation aims to follow the same semantics of file globs (when thinking of passes as directories). A * selects all subdirectories (subpasses) within a directory (group), and (with some implementation constraints) ** matches one or more hierarchy levels at once. Some random examples:

root                y.b.v.option    y.*.option    y.**.option   **.option
 |- x                    -              -             -             X
 |- y (group)
 |  |- a                 -              X             X             X
 |  |- b (group)
 |  |  |- u              -              -             X             X
 |  |  |- v              X              -             X             X
 |  |  '- w              -              -             X             X
 |  '- c                 -              X             X             X
 '- z                    -              -             -             X

The only differences with glob are that directories (passes) either have subdirectories (subpasses) or files (settable options), and globbing the names of the files (options) makes no sense, so those cases aren't handled. Also, y.**.v.option would logically select only y.b.v, but currently throws an exception, because ** is only supported as the last non-option element, because something made that annoying to implement, so for the sake of efficiency I didn't yet.

set_option_recursively just does set_option with a **. prefix. It's meant to be a less-cryptic alternative for something you might do relatively often, like c.set_option_recursively('output_prefix', 'some/output/folder/%N') to change the output directory for all passes at once.

One thing I do feel is missing here still is a way to select options based on pass type, so for example to set an option only for all clifford optimizers in the pass list, even if a similarly named option might exist for other pass types at some point as well. This also just didn't make the cut because I was trying to restrict myself to easy-to-implement stuff.

Note finally that the user isn't supposed to read the API reference first or even at all, there's supposed to be a user manual in front of it that guides them through basic usage. I'll concede that the API class documentation is very sparse right now and could be expanded, and that there are a lot of functions in the API reference that may not be useful right now, but just keep in mind that an API reference doesn't aim to teach someone how to use the corresponding library, in the same way that a dictionary doesn't aim to teach someone how to speak the corresponding language.

jvansomeren commented 3 years ago

I am confused about documentation.

What is a user, what is a developer?

What is a user? OpenQL is a compiler. So a user just uses it to compile a quantum program. But in doing so can set/unset options, add/delete passes, anything that the API allows. But also, the user must be able to understand what these mean, what a pass does and what the effect is when he adds/deletes passes. Of that the fundament is the IR. Therefore, the user documentation must also explain the structure and meaning of this IR, not only as external human readable thing but also as internal thing, because the external readable IR adds syntax and such which obscure the internal IR. This internal IR need not be described in terms of its actual implementation because that adds C++ details which again may obscure the meaning and clarity of the internal IR: it is about meaning, not syntax.

Anything not in the API is the domain of the developer: writing a new pass, extending/updating the IR, changing/writing a configuration file, etc.

Now, a user as described above may be able to do more than a 'simple user' may want to know. So one may consider a 'simple user' to be one who does not add/delete passes, who relies on internal defaults of pass order, who uses OpenQL out of the box, and therefore does not need to understand the internal IR to the depth as required for the 'advanced user'. For the 'simple user' it suffices to describe the IR in terms of what gets into OpenQL and what comes out. So here the cQASM documentation may be a start, supplemented by the variations that it supports (with/without mapping, with/without timing, etc.).

I propose to explain more clearly the target audience and the relation of the chapters of documentation to those kinds of audience in 'Overview', at the start of documentation.

jvansomeren commented 3 years ago

get_custom_instructions => dump_custom_instructions gate(... condstring:str) -> none: without condregs? This API can be taken out. measure(...b0:Int)->none: specifies b0 to be float in the description

jvanstraten commented 3 years ago

When I say "user," I'm talking about what you describe as a "simple user." Everyone in DiCarloLab except Wouter falls into that category, so I'd say it's the majority of our users (although I don't actually know how many people work in DiCarloLab). The Quantum Inspire people as well as entirely new users, no matter their intention, would also fall into this category. IMO they need the following things, and only the following things, in "OpenQL basics":

This part is meant to be "read like a book;" it should be a tutorial that the user can follow to get a basic idea of what OpenQL is, how to get it installed, and how to get a baseline Python file up and running that works on their machine. But that also means it needs to be short and interesting for everyone; if you start with "you first have to know X and Y and Z," especially when that isn't necessarily true for what a particular kind of user wants out of OpenQL, no one will actually read it. The "more information" part, on the other hand, should not read like a book, but act as additional information for parts referred to in basics, should a particular user decide for themselves that they want to know more about a particular subject. No one is supposed to read everything in a reference.

In my mind, there should be roughly three sections (with target audiences):

The latter is mostly meta stuff for the project right now, but I was intending to stick the really in-depth things from what's currently in OpenQL basics about how the implementation works either in there or in comments inside the code. The not as in-depth things should go in the pass documentation (so its source is inside the code, it can be queried from within an interactive Python console, and pops up in the "Supported passes" section of the reference). In the basics section the mapper, scheduler, optimizers, etc should be introduced, but with no more detail than what you put in your OpenQL introduction slides.

The target audience etc. should definitely be described in the overview/introduction! No argument from me there. But I'd always only write that after having written everything else already.

Finally, you mention pointing basic users to the cQASM documentation, which has me very confused. That is very much reference information, and information that only works with context at that, since every "user" of cQASM (as in piece of software that includes libqasm) has its own way of interpreting the semantic tree generated by it. It's absolutely not libqasm's place to document that OpenQL discards timing information when you do basically anything except code generation, for instance (libqasm happens to be used by OpenQL, but it's certainly not a part of it). You'd first have to know that we're using it as an impromptu compiler IR and what that means before you can even begin to understand anything in that documentation and actually arrive at correct conclusions. The same thing applies for Quantum Inspire; I see the libqasm documentation more as documentation for the QI people who have to update their frontend and then have to update their documentation, than as something for the end user. Just like how C++ users don't tend to read the C++ language specificationl heck, it's not even publicly available.

So, the basics section should introduce cQASM as OpenQL uses it at some point in a clear and concise way, very clearly stating which subset OpenQL supports at the input and which subset it generates at the output, with a link to libqasm's documentation for further details at the end. There should also be a note there that the cQASM generated by OpenQL is not always actually valid cQASM, because it can't be right now (since cQASM 1.x does not support control-flow, and I'm 100% sure that the generator doesn't generate valid 1.1 syntax for classical constructs either yet, though it could probably be modified to do so), there is the whole skip/wait debacle that still needs to be dealt with, it's going to use custom gate names rather than only cQASM's default gateset, the operand lists may be different (measure insns getting their bit result register made explicit, crk being converted to cr, the angle op of cr not even being printed because it's not in the list of insns with an angle) etc., and we don't just want to throw an error when unrepresentable IR constructs are used, because it also serves as debug output.

jvanstraten commented 3 years ago

I'm going to be AFK (driving) for the rest of the day, so I'll respond again for new stuff that pops up tomorrow.

jvansomeren commented 3 years ago

In this light we might add to the API functions an annotation "for advanced users only", after having defined/introduced "advanced user" in the introduction, or, more preferably, to the classes of those APIs. Because "user" == "people using API" is not true anymore otherwise. Note that Miguel et al explicitly asked for modularity because they wanted control over which passes would be included in the compiler; when they want (and must be) able to take such decisions independent of us, they must know of IR attributes such as "with mapping" and "with timing". Yes, and with cQASM I meant just enough to see the match between quantum gates in literature and those of OpenQL, not syntax, comment conventions, etc.

jvansomeren commented 3 years ago

I wrote:

I propose to undocument pass groups or to take them out of the implementation until we have a clear use case for them

I don't mean to discard and forget them. From my CoSy background I see several missed opportunities (or didn't see them yet) and don't want to get into a backward compatibility issue when we would want to copy some of CoSy's engine design into OpenQL's pass group and option design. The pass groups are about 80% CoSy's engine classes and some simple additions could turn that into 120%; that +20% because we encountered deficiencies in CoSy as well in the end such as lack of dynamics wrt pass management that are already in OpenQL. So the easiest would be to keep them out of the documented interfaces. And then I would like to:

This requires a face2face meeting, to save time.

jvansomeren commented 3 years ago

Regarding Topology::sort_neighbors_by_angle, it belongs to Topology in my opinion. But it is a thing that is not mature yet. It has to do with subsetting the qubits. So could be used to define cores or particular sections of qubits with particular functionalities. In this case, the section contains those that are on the shortest path between two given qubits. But a section may have attributes like being convex (any shortest path between two qubits in the section only contains qubits in the section), being connected. In the current case, the space of connections is planar, i.e. connections only cross in qubits (nodes). With non-planar grids, the spaces become higher-dimensional. Fully connected grids have the highest dimension. Reading this, I fear that some of the ibm grids are non-planar and that thus this sorting doesn't work for those grids, i.e. we have a bug.

jvansomeren commented 3 years ago

The cc_light default config file has a wrong cnot decomposition:

"gate_decomposition": {
        "cnot %0,%1" : ["ry90 %0","cz %0,%1","ry90 %1"]
    }

It should be (note the ry90 %0 to ym90 %1 gate change):

"gate_decomposition": {
        "cnot %0,%1" : ["ym90 %1","cz %0,%1","ry90 %1"]
    }
jvansomeren commented 3 years ago

Option generate_code is still used by the multi-core branch. When no, the final code generation is not done. Option mappathselect with value borders doesn't follow the borders of the chip but of the search space provided that the chip's coupling graph is planar. Option maptiebreak 's first/last values assume that the coupling graph is planar and neighbors were sorted; and assume knowledge on how the alternatives are generated then (first is left-most alternative with 2q near target, last is right-most alternative with 2q near source). Option use_default_gates: TODO must be moved to TODO section of documentation. Option write_qasm_files: end of sentence is incorrect.

jvansomeren commented 3 years ago

With the visualizer, radius etc is in pixels. Isn't a pixel a variable thing, i.e. can it be that the configuration needs to be updated for different pixel density?

jvansomeren commented 3 years ago

With the pass cQASM reader, the uninitiated reader doesn't understand the relation between lib qasm and cQASM reader.

jvansomeren commented 3 years ago

Several options have a file name string as value: cqasm_file, gateset_file, output_prefix, and probably several more. It is not clear how these file name strings are interpreted: are these absolute path names, are these relative to the installation root, are these relative to the current directory? Is there something to control this?

jvansomeren commented 3 years ago

Mapper pass: I would like to discuss what it does and how it can be improved. The lookahead_mode is troubled by the interaction with recursion and that only subsets of gates (2q gates, nonNN 2q gates) are relevant in lookahead; of course 1q gates and NN 2q gates can be immediately mapped. Perhaps the recursion should be taken out of the documentation and the lookahead_mode be reduced to a mode without recursion.

Without recursion, the lookahead_mode option implies: no: just map the gates in the order of the circuit; other modes assume a dependence graph and avlist; a 1q gate or a NN 2q gate maps trivially; map a nonNN 2q gate by generating alternatives for this nonNN 2q gate and selecting the best 1qfirst: map the 1q gates before any 2q gates in avlist until only 2q gates remain; then map one, i.e. the most critical 2q gate by generating alternatives for this nonNN 2q gate and selecting the best noroutingfirst: map the 1q gates and NN 2q gates in avlist until only nonNN 2q gates remain; then map one, i.e. the most critical nonNN 2q gate by generating alternatives for this nonNN 2q gate and selecting the best all: map the 1q gates and NN 2q gates in avlist until only nonNN 2q gates remain; then generate alternatives for all nonNN 2q gates and map that nonNN 2q gate corresponding to the best alternative selected.

path_selection_mode: not chip border but search space border, and only if coupling graph is planar

recurse_on_nn_two_qubit: this is the option interfering during recursion with lookahead_mode; this one and the following recursion options could be suppressed from documentation.

reverse_swap_if_better: its implementation may even be exactly wrong; and it depends on swap decomposition on the particular target, so perhaps it should be made more clear in which cases it does something and what it does then, also to make it independent of the target

tie_break_method: first and last depend on knowledge of alternative path generation; and taking the first/last is then only deterministic when the coupling graph is planar

jvansomeren commented 3 years ago

Resources and the new way of specifying resource constraints, reading the documentation.

This made the resource specification more powerful: more types of resources can be described.

Regarding Instrument resource, does the detuned_qubits resource match the description of general operation in the Instrument resource section? A mw and a flux don't need to have the same duration for running in parallel, nor need to start in the same cycle. The instrument in this case is the particular frequency choice for a qubit: default or detuned. Mw needs default; some flux need detuned on a qubit defined by the flux operand pair. Qubit exclusive use is assumed (other instrument). The section reads as an introduction to resources and is very useful in this respect, so should be kept but must be made accurate. I propose to add that this is a rationale/example/motivation, not a specification; or to add other types of resources (such as detuned_qubits) that are somehow different must must be supported as well. Or that the if and only if before the first bullet list is updated.

Key here is they use the same instrument function;, not the other ones. For a 1q mw and a 2q flux to be in parallel regarding the detuned_qubits resource, they must not both use the same instrument object. In the detuned_qubits example config, the 6 objects model the 6 qubits where a conflict could arise. The 1q and 2q select the instrument object thru different size-specialized predicates. So same instrument function means: same instrument (object) in the set of a particular instrument type.

Regarding the configuration structure: a required condition for an incoming gate to match a predicate is:

its instruction set definition object has values for all keys specified; When one forgets to add a particular key in a gate specification (e.g. "mw" as "type" in "x45"), it will never be matched; could this go unnoticed somehow? If so, consider checking this somehow. I don't have an idea now.

What could be emphasized more is that there always are two gates for which one wants to know whether they can be in parallel. For each such pair one checks all resource types. When for one resource type, both gates use (select to/match) the same instruction object, they cannot be in parallel. This operational definition is somehow assumed.

I don't understand "exclusive". Or, I don't understand the function section. Or, I don't understand: a list of gate keys as specified in the structure above

The original resource specs didn't support three-or-more-qubit gates because the underlying architectures don't support them. So why is their support now in?

Perhaps suppress the multi-core channel resource since it is not mature yet. There are many implicits here (deriving core index from qubit index, full core connectivity) which need ability to be specified. The documentation here is not ready yet either. The channel resource would only be allocated by inter-core-gates, a subset of 2q gates, each such gate allocates two channels, one in each core. I don't understand: This is mostly for compatibility with the original channel resource, which didn’t check for this.

jvansomeren commented 3 years ago

With the Release procedure, I am missing the writing of Release Notes. This is a summary of what has changed, what is new, what is incompatible and gives recipes how to overcome the latter. It also summarizes what is deprecated from now on and will be removed in a later version. It need not be a lot of text but should refer to the necessary texts in the documentation; it is an entry-level document.

jvansomeren commented 3 years ago

In C++ coding conventions, the section Namespaces appears twice. Regarding virgin constructors and an init method, I'm still curious why this was (or why I thought this was) needed in the mapper.

jvansomeren commented 3 years ago

I concluded with reviewing the PR documentation (as generated from within the docs directory).

jvanstraten commented 3 years ago

Up to https://github.com/QE-Lab/OpenQL/pull/405#issuecomment-831175809:

In Python API, compilation strategy is mentioned, a default compilation strategy which not be good enough and that you can change it. Only in class Platform it is described (with the compiler config) while it should be described in class Compiler at least.

Lazily added a single sentence description of what it's for (bunched in with 94a69be). This kind of thing is more something for user-manual style docs than API docs anyway.

(discussion about pass groups)

Removed all traces of pass groups that I'm aware of from the API based on QL_HIERARCHICAL_PASS_MANAGEMENT in config.h.template. I didn't want to remove everything completely, because I had to change a lot of docstrings to make everything sane again.

get_custom_instructions => dump_custom_instructions

This is an old API, not one that I added, so for backward compat it should stay in. I've added a dump synonym for it and deprecated the get version though.

gate(... condstring:str) -> none: without condregs? This API can be taken out.

It's for the always/never variants, and mirrors the internal function signature. Leaving it in allows gates to be added procedurally using gate(... condstring, *condregs) regardless of condition type (or lack thereof). Also, it's rather annoying to take it out because it's generated from C++'s optional argument system.

Also fixed a few compilation instruction things found by Quinten.

jvanstraten commented 3 years ago

Up to https://github.com/QE-Lab/OpenQL/pull/405#issuecomment-832481890:

(sort_neighbors_by_angle stuff)

Replaced the FIXME with a TODO note referring to this comment, don't know what else to do with it for now.

(CC-light CNOT gate decomposition)

Fixed, but note this changed golden files. It wasn't broken during refactoring.

Option generate_code is still used by the multi-core branch. When no, the final code generation is not done.

It's defunct because the QISA code generation pass was removed, so regardless of whether it's yes or no, final code generation is not done.

Option mappathselect with value borders doesn't follow the borders of the chip but of the search space provided that the chip's coupling graph is planar.

Updated.

Option maptiebreak 's first/last values assume that the coupling graph is planar and neighbors were sorted; and assume knowledge on how the alternatives are generated then (first is left-most alternative with 2q near target, last is right-most alternative with 2q near source).

Updated.

Option use_default_gates: TODO must be moved to TODO section of documentation.

I'm not sure what you mean by this, and in general the option documentation is generated, so it's not really possible to add RST directives and whatnot to them without making everything vastly more complicated.

Option write_qasm_files: end of sentence is incorrect.

Fixed.

With the visualizer, radius etc is in pixels. Isn't a pixel a variable thing, i.e. can it be that the configuration needs to be updated for different pixel density?

I highly doubt that the CImg library is aware of pixel density and GUI scale, and if not, it's not really something that can easily be fixed. Internally CImg just makes a bitmap image (which it can also write to a file as such) which logically uses pixel units, and eventually makes a window that displays this bitmap. It's no more intelligent than that. If this is/becomes a problem maybe something can be tacked on to fix it later, but that'd be a feature request that really shouldn't be bunched up in a refactoring commit.

With the pass cQASM reader, the uninitiated reader doesn't understand the relation between lib qasm and cQASM reader.

Added short explanation of relationship between the two, with reference to libqasm's ReadTheDocs page.

(filename relativeness)

Anywhere this is not specified, filenames must either be absolute or relative to the current working directory as tracked by the OS. IMO this is just how computers work, so I really don't feel like this is something worth repeating everywhere...

We could quite easily change the behavior since all file I/O is wrapped by utils, but I don't really see why in this case? I suppose I could see it be useful when these option values are specified in a compiler configuration file, but any implementation for that would have to be really contrived, because it would mean there are special cases for certain options when they're specified in a certain way...

jvanstraten commented 3 years ago

For https://github.com/QE-Lab/OpenQL/pull/405#issuecomment-832506285:

Mapper pass: I would like to discuss what it does and how it can be improved. [...]

Changing the functional implementation of the mapper should be a separate issue/PR, especially at this point in the process.

Without recursion, the lookahead_mode option implies: [...]

Updated option docs accordingly.

path_selection_mode: not chip border but search space border, and only if coupling graph is planar

Already updated along with the global option.

recurse_on_nn_two_qubit: this is the option interfering during recursion with lookahead_mode; this one and the following recursion options could be suppressed from documentation.

As I mentioned in an earlier comment, the option documentation (including pass option documentation) is generated from the options exactly as they exist in the sources and thus include everything, and I firmly believe this is a good thing. That's what it's a reference for. So, instead of surpressing, I added notes stating that these are advanced and unstable options, for recurse_on_nn_two_qubit including that it affects lookahead_mode in a currently-undocumented way.

reverse_swap_if_better: its implementation may even be exactly wrong; and it depends on swap decomposition on the particular target, so perhaps it should be made more clear in which cases it does something and what it does then, also to make it independent of the target

I feel like the docstring I added to it already explained this, but I expanded upon it to make it more clear. IMO it's up to the user to define the composition such that it complies with the convention.

I also more properly documented the general behavior of the mapper, and noted there what the requirements on the gate/decomposition rule definitions in the platform are.

jvanstraten commented 3 years ago

For https://github.com/QE-Lab/OpenQL/pull/405#issuecomment-832571528:

Regarding Instrument resource, does the detuned_qubits resource match the description of general operation in the Instrument resource section? [...]

You're right, good catch... Also, reading your comment and rereading the docs for this section, it seems a bit weird that the instruments are not by qubit (i.e. whether an instrument is detuning the qubit or not) but by edge. I think the latter is sound though (and was probably easier to generate from the old structure)...

I remember that when I worked on generalizing the instruments before (in a more advanced way rather than just ad-hoc what seemed to be needed to model the CC-light resources more generally) I modelled instrument usage as a particular function per cycle, rather than using cycle ranges. The alignment requirement could then be modelled by using a different function for the first, last, and any further cycles for all gates, while the behavior otherwise would be that alignment is not required at all. This is probably why I didn't realize that this (simpler) implementation doesn't work that way.

Unfortunately that means that the reservations tracker thing I made is also not as general as I would have liked it to be, so I should probably redo that (this doesn't necessarily take a lot of time, maybe a day or two). So I'll work through the other issues first.

Regarding the configuration structure: a required condition for an incoming gate to match a predicate is: [...]

Yes, it could go unnnoticed, because by definition none of these keys are required. But I can list a plethora of other issues similar to or IMO more important than this one with the current platform configuration system; any typo anywhere can easily make a key not recognized, have its default be used instead, and not be caught because historically people have been dumping no-longer-used keys all over the place, so I can't just throw an error for unrecognized things.

However, users should ideally not be forced to specify repetitive things like this manually to begin with. That's what the platform preprocessing thing for architectures is for. If a particular platform requires that each gate has a "type" key and has requirements on what values it may map to, this should be checked there, not by the (generalized) instrument resource. Similarly, this logic may desugar more easily specifiable resources into instrument resource configurations.

What could be emphasized more is that there always are two gates for which one wants to know whether they can be in parallel. For each such pair one checks all resource types. When for one resource type, both gates use (select to/match) the same instruction object, they cannot be in parallel. This operational definition is somehow assumed.

I don't understand what you mean by this. Please rephrase.

I don't understand "exclusive". Or, I don't understand the function section. Or, I don't understand: a list of gate keys as specified in the structure above

Admittedly the section on how to specify the value for "function" is a bit confusingly written, although I'd argue that

The instrument function is configurable for each particular gate based on one or more custom keys in the instruction set definition of the platform configuration file. It's also possible to specify that there is only a single function (effectively, this removes the first condition in the list above), or to specify that all functions are mutually exclusive (in which case gates using the same instruments can never be parallelized).

in the behavioral description is good enough? Either way, I'll have to rewrite the docs when I update the resource to more properly support qubit detuning.

The original resource specs didn't support three-or-more-qubit gates because the underlying architectures don't support them. So why is their support now in?

I added it so specifying detuning via "3-qubit" CZ gates (i.e. with the detuned qubit(s) made explicit) could also be supported, and because it seemed like a reasonable generalization in general.

Perhaps suppress the multi-core channel resource since it is not mature yet. There are many implicits here (deriving core index from qubit index, full core connectivity) which need ability to be specified.

How qubits map to cores is a topology thing, and is documented (sparsely, I'll admit, but it's there) in the topology configuration description. Though the core_index = floor(qubit_index / num_cores) thing was missing from there.

The documentation here is not ready yet either. The channel resource would only be allocated by inter-core-gates, a subset of 2q gates, each such gate allocates two channels, one in each core. I don't understand: This is mostly for compatibility with the original channel resource, which didn’t check for this.

I don't understand what you mean. The documentation describes the resource as I (re)implemented, which is a basic generalization of what was there:

The "communication_qubit_only" augments the above when set, by ignoring qubit operands that are not communication qubits.

TODO:

jvanstraten commented 3 years ago

With the Release procedure, I am missing the writing of Release Notes.

It was very vaguely in there (though I called it a short changelog), but I expanded on it now and added a few other missing steps while I was at it. I was intending to update it while doing the actual release, so I hadn't looked at it much yet.

In C++ coding conventions, the section Namespaces appears twice.

Fixed.

Regarding virgin constructors and an init method, I'm still curious why this was (or why I thought this was) needed in the mapper.

Just because C++ is annoying with its member initialization syntax. There are ways around it, but it quickly becomes a lot more messy than it's worth. It's easier to just wrap a class member in utils::Opt or utils::Ptr when you can't easily initialize it before the constructor's function body; this lets you do everything that virgin constructor + init lets you do, but ensures that you can never use the object accidentally before initializing it (you'll get a runtime error if you do, stating that you're dereferencing an empty Opt or Ptr) and still lets you use RAII for the affected class where this issue doesn't exist.

I believe that's everything except for the functional limitations for the instruments and the still-missing user docs and tests for the new compiler API.