Add quil-plotting module #412
Conversation
|
| RED = "#ef476f" # Magneta | ||
| BLUE = "#3d47d9" # Palatinate Blue | ||
| GRAY = "#8a8b92" # Gray | ||
| NAVY = "#0d0d36" # Cetacean Blue |
There was a problem hiding this comment.
Are these colors configurable? They should be for accessibility.
There was a problem hiding this comment.
It's the Rigetti colors. They aren't easily configurable. Any suggestion how to make it so?
|
|
||
| [build-system] | ||
| requires = ["hatchling"] | ||
| build-backend = "hatchling.build" |
There was a problem hiding this comment.
I'm good with using hatchling here.
There was a problem hiding this comment.
How are these checkpoints used in tests? Since this is a pure Python module, I think a brief CONTRIBUTING.md explaining how to run and test this code would be valuable. I would also add a cargo-make Makefile with tasks to run the tests, at least.
There was a problem hiding this comment.
This is a mistake, will remove.
| "Operating System :: OS Independent", | ||
| ] | ||
| dependencies = [ | ||
| "numpy>=1.2.1", |
There was a problem hiding this comment.
Looks like the mistaken version of numpy, should be 1.21.0 right?
| color_by: str = "Channel Type", | ||
| runner_order: str = "Time (s)", | ||
| exclude_readout: bool = True, | ||
| normalize_by: Optional[str] = "Channel Type", |
There was a problem hiding this comment.
Instead of plain strings, can we make "categoricals" and the like enums?
There was a problem hiding this comment.
Yes, however I'm not sure if using enums with a pandas dataframe is entirely conventional. I'm not actually sure what best practice would be here.
|
|
||
|
|
||
| def deoxidize(num) -> complex: | ||
| """Remove Rust from a Number or PrefixExpression.""" |
There was a problem hiding this comment.
This mentions the Number variant, but doesn't handle it. Also, the evaluate method is available on any Expression, and raises an error if it cannot evaluate the expression to a complex number. So this could be simplified to a single line while keeping the same behavior. Since it is a single line, this function can likely be removed.
num.evaluate({}, {})There was a problem hiding this comment.
This doesn't seem to work on PrefixExpressions. Will revisit it though.
| sample_rate = frame["SAMPLE-RATE"].inner().to_real() | ||
| match name: | ||
| case "flat": | ||
| waveform_envelope = FlatWaveform(**parameters) |
There was a problem hiding this comment.
Each of the template waveforms have an into_iq_values method, any reason that isn't used here?
| elif isinstance(program, Program): | ||
| quil_program = program | ||
| else: | ||
| raise ValueError("Program must be a pyquil.Program or a quil.Program") |
There was a problem hiding this comment.
Hmm, should this package be aware of pyQuil's internal API? I think it would be better to remove this special case, and add a method to pyQuil's Program that can call into this package instead. That way, implementation details about pyQuil stay in pyQuil.
There was a problem hiding this comment.
No, removing this is in the TODO.
| calibrations = quil_program.calibrations | ||
| expanded_quil_program, source_map = expand_with_source_mapping(quil_program, include_pragmas=False) | ||
| blocks = expanded_quil_program.control_flow_graph().basic_blocks() | ||
| assert len(blocks) == 1 |
There was a problem hiding this comment.
ruff should raise a lint for this. In short, assert is removed from optimized Python code, and it's generally better to explicitly raise an error. Reference
The intention is to use the TemplateWaveforms (I believe this is mentioned in the TODO list and it's actually necessary to avoid a circular dependency with pyquil). At the time of writing, I don't think this feature was released so that's why I made due with the pyquil ones. As for documentation, what do you have in mind? Should this integrate with the quil-rs documentation? |
No description provided.