change simpleexpression for equality handling #828
Conversation
Nomos11
commented
May 31, 2024
•
Nomos11
commented
- Release a working version that can efficiently translate increments of constant hold commands on HDAWG
- Test and fix for post != pre dependency state
- Test and fix for offby one jump back increment
- Test and fix for resolution dependent jump back
- Release a version that generates dynamic stepping through the command table
- Release a version that generates linspace playHold commands
- Release a version with linspace scaled arbitrary waveforms
Test Results 6 files ± 0 6 suites ±0 4m 42s ⏱️ + 1m 1s For more details on these failures and errors, see this check. Results for commit def9369. ± Comparison against base commit 49d9a2d. ♻️ This comment has been updated with latest results. |
|
Does SimpleExpression implement the expression protocol? If not we should definitely mention that in the docs. |
|
Not an expert there but does not seem so; moving it around was also just to avoid some circular imports at some point (I'm not even sure it's necessary anymore or if I removed such changes again), it can also be moved further. |
|
This PR has a lot of unrelated changes |
|
yes; it's evolved to collecting random changes to merge into some other side branch to work on, not into master; thought that it would not require spamming more PRs for that / just get general feedback on changes |
| @@ -88,7 +88,16 @@ def get_constant_output_channels(self, input_channels: AbstractSet[ChannelID]) - | |||
|
|
|||
| class ChainedTransformation(Transformation): | |||
| def __init__(self, *transformations: Transformation): | |||
| self._transformations = transformations | |||
| #avoid nesting also here in init to ensure always flat hierachy? | |||
There was a problem hiding this comment.
This is the job of the function chain_transformations
There was a problem hiding this comment.
yes; basically copied over that code since the function instantiates a new ChainedTransformation.
I wasn't sure if ChainedTransformation is called directly anywhere instead of being instantiated with .chain, so I did the check again in the init function. If the class is not supposed to be initialized directly, it could also be removed again.
|
|
||
| def _root(self): | ||
| return self._stack[0] | ||
|
|
||
| def _get_rng(self, idx_name: str) -> range: | ||
| return self._get_ranges()[idx_name] | ||
|
|
||
| def inner_scope(self, scope: Scope) -> Scope: | ||
| def inner_scope(self, scope: Scope, pt_obj: Optional['ForLoopPT']=None) -> Scope: |
There was a problem hiding this comment.
The program builder should not know about pulse templates. What is the reasoning behind passing this object here?
There was a problem hiding this comment.
this is all hacked in right now to see if anything works with what i intended to do.
reasoning here to keep the glue code out of the pulse_template._internal_create_program
There was a problem hiding this comment.
I would say things can be done more elegantly once one sees if the desired concepts work at all or a different approach has to be chosen altogether
|
|
||
| @dataclass | ||
| class LinSpaceArbitraryWaveformIndexed(LinSpaceNodeChannelSpecific): | ||
| """This is just a wrapper to pipe arbitrary waveforms through the system.""" |
There was a problem hiding this comment.
I guess this is no longer true
There was a problem hiding this comment.
yes, docstrings are not adjusted atm
|
|
||
|
|
||
| @abstractmethod | ||
| def _compare_subset_key(self, channel_subset: Set[ChannelID]) -> Hashable: |
There was a problem hiding this comment.
What is the reason for this method? The current implementation is worse than hash(wf.get_subset_for_channels(channel_subset)). If we need better subset analysis, we can improve waveform normalization.
There was a problem hiding this comment.
this was done before the compare_key thing was reworked, i believe.
but your code would take into account channel names, right? this is what i tried to avoid there to just compare potential sample output.
There was a problem hiding this comment.
this change however is also not relevant for the linspace thing and propagated to the branch from somewhere
