Parameters builder #1123
Parameters builder #1123
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1123 +/- ##
==========================================
+ Coverage 50.65% 51.53% +0.88%
==========================================
Files 63 63
Lines 2922 2994 +72
==========================================
+ Hits 1480 1543 +63
- Misses 1442 1451 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Minor considerations, but that's mostly what I had in mind.
In particular, the builder implementation (i.e. the content of .build() and the functions it is calling) is especially useful to initialize a platform from scratch.
I will try to use this myself (I have a use case with the testing I'll do for Qblox), and keep it as the core of a tutorial I may write later on.
All in all, let's refine the implementation, but concerning the substance, we could even merge as it is.
| if parameters is None: | ||
| return Platform.load(path, **hardware) | ||
|
|
||
| return Platform(**hardware, parameters=parameters) |
There was a problem hiding this comment.
This part I'm not completely sure about... maybe I'd limit to
| if parameters is None: | |
| return Platform.load(path, **hardware) | |
| return Platform(**hardware, parameters=parameters) | |
| return Platform.load(path, **hardware) |
and avoid parameters as input.
My idea is that create_platform is often the function used by the backend and high-level users (they are calling it by name).
Instead, for platforms' creators, we may just expose the _load() function (dropping the _, of course), such that you can create it using Platform() yourself.
The main benefit would be to keep the two functions simpler and modular (otherwise create_platform() would cater for three distinct situations, and essentially wrap _load() when you need just that).
| InstrumentMap = dict[InstrumentId, Instrument] | ||
|
|
||
|
|
||
| class Hardware(TypedDict): |
There was a problem hiding this comment.
I'm not fully convinced with this name. Though I used it in the first place...
But I currently have no better proposal.
| InstrumentMap = dict[InstrumentId, Instrument] | ||
|
|
||
|
|
||
| class Hardware(TypedDict): |
There was a problem hiding this comment.
The TypedDict is perfect to annotate **kwargs, or other places where you're bound to use dictionaries anyhow.
However, the moment it gets part of a serialized hierarchy (cf. ParametersBuilder below), it may be worth to make it a full-fledged Model.
| class ParametersBuilder(Model): | ||
| """Generates default ``Parameters`` for a given platform hardware | ||
| configuration.""" | ||
|
|
||
| hardware: Hardware | ||
| natives: set[str] = Field(default_factory=set) | ||
| pairs: list[str] = Field(default_factory=list) | ||
|
|
||
| def build(self) -> Parameters: |
There was a problem hiding this comment.
Making it a builder was just instinctive, having in mind something like this.
However, while the builder pattern may be useful to break down a constructor of something complex (essentially, currying it), in this case it is sufficiently simple that we could keep it as a standalone function, or even a constructor of Parameters (but maybe I'd avoid this last option, to keep it more modular).
The equivalent function would read sufficiently smooth anyhow:
def init_parameters(hardware: Hardware, natives: Optional[set[str]] = None, pairs: Optional[list[str]] = None):
...In any case, I'm not strongly against the builder as well.
It was just to acknowledge that, reading the final result, in the end you were right, and it seems unnecessary (but not necessarily bad).
| hardware = {"instruments": instruments, "qubits": qubits, "couplers": couplers} | ||
| try: | ||
| parameters = Parameters.model_validate_json((path / PARAMETERS).read_text()) | ||
| except FileNotFoundError: | ||
| parameters = ParametersBuilder(hardware=hardware).build() | ||
|
|
||
| return cls(name=name, parameters=parameters, **hardware) |
There was a problem hiding this comment.
In this case, maybe I'm plainly against. The code is still simple enough, so that's not a big deal, but the load function would then attempt doing two different things, which may complicate (slightly, but significantly) the workflow of the code based on it.
Instead, I would keep Platform.load() with the former behavior, and leave the responsibility to the user to handle the missing parameters file case. Which should only happen during platform's creation, and it's sufficiently simple anyhow, i.e.
params = ParametersBuilder(hardware=hardware).build()
Platform(instruments=instruments, qubits=qubits, couplers=couplers, parameters=params)
Implements the
ParametersBuilderdiscussed with @alecandido last week. The main change, from a usability point of view, is thatPlatforms can now be defined without writing the correspondingparameters.json. In that case, a default parameter configuration will be autogenerated, using the newParametersBuilderobject.The generated
Parameterswill have configs with zero values for all channels defined within thecreate_method. They will also have default native gates (also with zeros) for all gates specified asnativesin theParametersBuilder. Note thatParametersBuildermakes some assumptions about the config kinds and channels that native gates play on, which the user may need to change in practice for things to work properly on hardware. The updates can be done either directly in Python usingParameter.replaceAPI or dumping to JSON and updating manually.@alecandido let me know if this is what you also had in mind, because I am not sure. Then we can add some examples in the documentation of how this can be used to simplify platform creation (avoid copy-pasting, etc.).