Waveform improvements#485
Conversation
129efe4 to
536f275
Compare
536f275 to
fc077e4
Compare
|
Co-authored-by: Jessica Jeng <mingyoungjeng@gmail.com>
asaites
left a comment
There was a problem hiding this comment.
I've made some comments, but think I need to spend a bit more time with the code first.
In general, my concern is that the abstraction level is grander than necessary for the goals, and that it makes the code particularly challenging to follow (and perhaps use and maintain).
What would help a lot for me for working out how this impacts Python users/what suggestions I can make for the PyO3 usage: could you update some or all of the Python tests that are currently broken?
|
I ran into an unexpected issue while reviewing this PR. Please try again later. |
asaites
left a comment
There was a problem hiding this comment.
Thanks for the hard work here; I really appreciate the documentation changes and the comprehensive the Python tests. I do have a couple of minor comments on a few things that are more style than substance, I think.
I'm going to look into what's up with CI right now, as there are a lot of failures. I think they may require some customization to the bindings' linter to handle the macros you've added.
| // U+005B is `[` (i.e., LEFT SQUARE BRACKET), but we have to hide it with an escape so as | ||
| // not to confuse our homegrown PyO3 linter script (`quil-rs/scripts/lint-quil-rs.py`). |
|
I pushed a couple commits to this branch to address the CI failures related to linting issues. Mostly, they were false positives: the new waveform macros generating classes, so the linter needed to be taught how to interpret the macro invocation to connect it to the exported classes. There was one legitimate issue: It looks like there's a new |
|
The |
I know, I know.
asaites
left a comment
There was a problem hiding this comment.
I'm still working through some of this, but overall things are looking pretty good. I have a few general comments at the moment and figured I'd submit them now, but I'll submit more soon.
| /// ) -> Waveform[OtherReal, OtherComplex]: | ||
| /// ``` | ||
| /// | ||
| /// 3. Methods may have a different type annotation placed on `self`. For instance, this can |
There was a problem hiding this comment.
Since I'm pretty sure that can only be useful on an @overload, I suspect this case would be better covered by using pyo3-stub-gen's features for specifying those, but doing so will require stepping up to the latest rigetti-pyo3 version, and overall the change is going to take a little more time to get all the pieces together (particularly around the linter and CI). I don't see any reason for this to block this PR, but it might mean all this work here would be better suited to just writing inline using existing pyo3-stub-gen features.
There was a problem hiding this comment.
The use case for explicit self is not just for overloads – I use it here to generate, for instance,
def iq_values_at_sample_rate(
self: BuiltinWaveform[builtins.float, builtins.complex],
common: CommonBuiltinParameters[builtins.float, __T],
sample_rate: builtins.float,
) -> IqSamples:
...
That method isn't meaningful to call unless the BuiltinWaveform is fully concrete like that. I use a lot of pyo3-stub-gen's overload features, but this is orthogonal.
| stop, | ||
| step, | ||
| slicelength, | ||
| } = indices.indices(self.sample_count() as isize)?; |
There was a problem hiding this comment.
Can't self.sample_count() >= isize::MAX?
There was a problem hiding this comment.
Ah, no it can't – Rust (and I believe also Python) guarantees that any single allocated object has size at most isize::MAX (see here). This comes from LLVM and I think is a pretty standard limitation. I'll add a comment.
There was a problem hiding this comment.
Hm, this doesn't work right in the Flat case, though. I don't think it would be unreasonable to not support Flat waveforms of that size, but I'll see if I can simplify this.
There was a problem hiding this comment.
PyO3 doesn't seem to offer enough support for this (https://pyo3.rs/main/doc/pyo3/types/struct.pyslice), so I simply added an informative error
asaites
left a comment
There was a problem hiding this comment.
Thank you for the excellent additional documentation, and the hard work you've put in to making the Python interface (and stubs!) fit together better. Overall, I think this looks pretty good, but I am a little skeptical of some of the added complexity around bits of the Python pieces. I don't see anything there that I think blocks the PR, though. We might need some cleanup for it when we upgrade to the newer versions of the stub generator and PyO3.
I've made a few comments on some aspects that I think should improve for the sake of "that's a known performance hit, and the more performant version is just as clean". Other than that, I saw a couple minor issues that should be addressed before pulling it in, but I don't think it needs to languish much longer.
| version = "0.36.0" | ||
| edition = "2021" | ||
| rust-version = "1.83" | ||
| rust-version = "1.93" |
There was a problem hiding this comment.
Apparently versions 1.87-1.96 have a miscompilation bug (in LLVM).
There was a problem hiding this comment.
Oof, good to know. It sounds like it's not a serious enough miscompilation bug to avoid setting our MSRV for, though, per this comment:
- It's only on x86-64 (we should let e.g. macOS users build this library on any version)
- It can only cause harm by unexpectedly crashing (there's no security or misbehavior risk)
- It's hard to observe on Rust versions <1.97, and 1.97 got a patch release to fix this.
|
Commit 8c03dab is not a response to review – when my internet died, I needed to build the docs locally, and this revealed a number of warnings from |
| stop, | ||
| step, | ||
| slicelength, | ||
| } = indices.indices(self.sample_count() as isize)?; |
There was a problem hiding this comment.
Ah, no it can't – Rust (and I believe also Python) guarantees that any single allocated object has size at most isize::MAX (see here). This comes from LLVM and I think is a pretty standard limitation. I'll add a comment.
| let fix_negative_index = |i: isize| -> PyResult<Option<usize>> { | ||
| if i < 0 { | ||
| let i = i.checked_add_unsigned(sample_count).ok_or_else(error)?; | ||
| Ok(usize::try_from(i).ok()) | ||
| } else { | ||
| // Positive `isize` fits in `usize` | ||
| Ok(Some(i as usize)) | ||
| } | ||
| }; |
There was a problem hiding this comment.
Thank you for the suggestion! The simpler behavior you had here allowed me to figure out how to express this much more directly as clamping to [0, sample_count].
| stop, | ||
| step, | ||
| slicelength, | ||
| } = indices.indices(self.sample_count() as isize)?; |
There was a problem hiding this comment.
Hm, this doesn't work right in the Flat case, though. I don't think it would be unreasonable to not support Flat waveforms of that size, but I'll see if I can simplify this.
| stop, | ||
| step, | ||
| slicelength, | ||
| } = indices.indices(self.sample_count() as isize)?; |
There was a problem hiding this comment.
PyO3 doesn't seem to offer enough support for this (https://pyo3.rs/main/doc/pyo3/types/struct.pyslice), so I simply added an informative error
| /// ) -> Waveform[OtherReal, OtherComplex]: | ||
| /// ``` | ||
| /// | ||
| /// 3. Methods may have a different type annotation placed on `self`. For instance, this can |
There was a problem hiding this comment.
The use case for explicit self is not just for overloads – I use it here to generate, for instance,
def iq_values_at_sample_rate(
self: BuiltinWaveform[builtins.float, builtins.complex],
common: CommonBuiltinParameters[builtins.float, __T],
sample_rate: builtins.float,
) -> IqSamples:
...
That method isn't meaningful to call unless the BuiltinWaveform is fully concrete like that. I use a lot of pyo3-stub-gen's overload features, but this is orthogonal.
This PR improves the support for working with actual waveform data in
quil-rs. In particular, these means there are a few categories of changes:There is now a single type for each sort of builtin waveform, e.g.
DragGaussianfordrag_gaussianthat, thanks to the usage of generic parameters, can represent any ofDragGaussian<Concrete>) – the only thing we could work with previously;DragGaussian<Syntactic>); orDragGaussian<Pythonic>).The different waveform types are handled uniformly through a mix of type and macro machinery, which is exposed in a way that hides most of the complexity. While the special type parameters are still present, the user does not need to be aware of, for instance, some of the higher-kinded type machinery that allows for easy definition of waveform sampling.
Waveform sampling is defined in a way that distinguishes between flat waveforms and waveforms that become an actual list of samples by using the new
IqSamplestype. All built-in waveforms can be sampled to produce one of these results.Waveform definitions and sampling are ideally easy enough to write that the waveform experts can dip in, write just the code they need by looking at existing patterns, and dip out again, without having to actually deal with any of the machinery that I put in place. (This is, in fact, the reason for a lot of the machinery!)
There is a uniform presentation for built-in waveforms that factors out the common parameters of
duration,scale,phase, anddetuning(into the typeCommonBuiltinParameters), which can be applied to any of the built-in waveforms. This is an extension of functionality; it was not previously possible to detune flat waveforms, for instance.The
stub_genbinary has been extended to be able to edit the generated type stubs; these edits are specifically to work with generic parameters (e.g., to make classes generic). This is admittedly kind of hacky, but is a necessary workaround to provide the right Python interface to waveforms and sampling given that py-stub-gen doesn't support generic types.As an extension for very specific needs, there's also support for "partial" waveforms (e.g.,
DragGaussian<Partial<Concrete>>) that may be missing some of their parameters and which can be sampled anyway; this produces either a placeholder indicating the shape of sample that will be produced if all the parameters are filled in, or an actual sample result.