Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linspace bug follow up with no bugs #862

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

terrorfisch
Copy link
Member

@terrorfisch terrorfisch commented Jun 21, 2024

This is a rebase of #858 with includsion of VM based test.

The program and commands produced by the original code yield the correct result.

Increment(channel=0, value=0.01, dependency_key=DepKey(factors=(10000000,))),
Wait(duration=TimeType(1000000, 1)),
LoopLabel(idx=2, count=9),
#also here, an increment 0 may be helpful (at least to be able to force).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The increment is not emitted due to an optimization. Why would it be helpful?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go further and replace loops that are constant by waits

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for wait it's not helpful. Just for other things, like WF scale, the program would need to know about the register it has to choose, which may work smoother if the corresponding Set command with the relevant DepKey is directly in front of it. Or maybe not, I dont' know.

Copy link
Member Author

@terrorfisch terrorfisch Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a scaled play should have a dependency key.

Copy link

Test Results

    3 files      3 suites   3m 9s ⏱️
1 198 tests 1 137 ✅  61 💤 0 ❌
3 594 runs  3 411 ✅ 183 💤 0 ❌

Results for commit 98d2a01.

#however, i think this is what should happen (maybe also with an additional "Set" before,
#which might cause complications if omitted in other contexts like AWG amplitude:
#LoopLabel(idx=0, count=10),
#Set(channel=0, value=-1.0, key=DepKey(factors=(10000000,))),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This set statement is not required. because it is the same in every iteration and can be lifted out of the loop. Why should it happen? This is exactly one intended case the post != pre condition is supposed to detect and optimize.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on further consideration, I think the issue that I saw when I first thought about this - that for long sequences that get repeated, this whole block is basically written twice in the sequence code, first for initial Setting, then forlooping n-1 times - is less of an issue overall if (what I did some time after that) e.g. for the HDAWG the entries for the CT can be recycled for the second code block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the thing is that it is required to do this twice because the initial state changes. I brooded over this quite a while I remember :D But it might still be that I am wrong...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don‘t think you‘re wrong; just in the very pathological case of repeating a sequence of 1000 pulses, where one element is just „Set“ with a Set command, one may want to prefer to have this codeblock only once and does not want that perceived optimization of omitting the Set command but having to write the repeated sequence twice.
But I don‘t think this is very relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants