Add pulse selection machinery and manual pulse patterns #417
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #417 +/- ##
==========================================
+ Coverage 72.87% 73.18% +0.30%
==========================================
Files 35 35
Lines 6043 6146 +103
==========================================
+ Hits 4404 4498 +94
- Misses 1639 1648 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| pulse IDs in every train. | ||
| """ | ||
|
|
||
| if isinstance(train_ids, TrainData): |
There was a problem hiding this comment.
If isinstance(train_ids, TrainData) is False, the train_ids can have any type*. It seems to me that this is not the desired behavior, is it?
* If we run a type checker, which AFAIK we don't, then any type is clearly any type that matches np.typing.ArrayLike but still, this means a list of strings would be okay.
| pulse_ids = np.arange(pulses_per_train) * pulse_period \ | ||
| + pulse_id_offset | ||
|
|
||
| return cls.from_constant_pulses(train_ids, pulse_ids) |
There was a problem hiding this comment.
Train IDs can be any object that implements a __len__ method.
| train_sel) | ||
|
|
||
| else: | ||
| train_sel = np.s_[:] |
There was a problem hiding this comment.
Super duper minor thing, any reason to use Numpy here when slice(None) suffices? That's what np.s_[:] returns anyway.
| # Convert by_index to a regular slice. | ||
| pulse_sel = pulse_sel.value | ||
|
|
||
| if isinstance(pulse_sel, by_id): |
There was a problem hiding this comment.
Okay, here I'm a bit confused because isinstance(by_id([100, 102, 104, 106]), by_id) evaluates to True but does not have a .value attribute nor does it have a .value.start etc. (by extension).
Am I missing something here?
| def slice_pulses(pulse_ids): | ||
| return pulse_ids.iloc[pulse_sel] | ||
|
|
||
| elif isinstance(pulse_sel, (list, np.ndarray)): |
There was a problem hiding this comment.
Here it explicitly allows a List for pulse_sel so comment above stands I think...
The existing
PulsePattern-based components can expose pulse patterns as described by data, but so far it was not possible to modify those patterns. Conceptually it seemed rather convoluted to track modifications from the data state. Independent of that, I meant to have someManualPulsescomponent for some time that exposes the same interface but allows to make up your own patterns. Turns out those two concepts actually are made for each other!This PR adds said
ManualPulsestype that simply holds any desired pulse pattern internally without any data reference, and then implementsselect_pulses(),deselect_pulses()andunion()on top of that - applying any such modification to aPulsePatterncomponent will always return aManualPulsesobject indicating it is no longer represents a pattern found in data.Furthermore, I tried to improve the documentation on these components with some introduction and examples.