[Bug] 'Pulse too short' on some sequences of pulses

[Slimane33]

The following code

def f(x): 
    reg = Register.square(3, 6)

    seq = Sequence(reg, Chadoq2)
    seq.declare_channel('ch0', 'rydberg_global')
    seq.declare_channel('ch1', 'rydberg_local')

    for i in range(2):
        pulse = Pulse.ConstantPulse(x * 4, 10, 0, 0)
        seq.add(pulse, 'ch0')
        for j in range(9):
            seq.target(j, 'ch1')
            pulse = Pulse.ConstantPulse(52, 0, (j+1)*10, 0)
            seq.add(pulse, 'ch1')

    seq.measure(basis='ground-rydberg')

    sim = Simulation(seq, sampling_rate=1)
    results = sim.run(nsteps=1000)

raises for f(43), f(44), f(45)

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-73-28c03cf16d82> in <module>
----> 1 f(45)

<ipython-input-72-15951c01fd5c> in f(x)
     13             seq.target(j, 'ch1')
     14             pulse = Pulse.ConstantPulse(52, 0, (j+1)*10, 0)
---> 15             seq.add(pulse, 'ch1')
     16 
     17     seq.measure(basis='ground-rydberg')

~/Desktop/unitaryhack/Pulser/pulser/sequence.py in wrapper(self, *args, **kwargs)
    109             verify_variable(x)
    110         storage = self._calls if self._building else self._to_build_calls
--> 111         func(self, *args, **kwargs)
    112         storage.append(_Call(func.__name__, args, kwargs))
    113     return wrapper

~/Desktop/unitaryhack/Pulser/pulser/sequence.py in add(self, pulse, channel, protocol)
    512         tf = ti + pulse.duration
    513         if ti > t0:
--> 514             self._delay(ti-t0, channel)
    515 
    516         prs = {self._phase_ref[basis][q].last_phase for q in last.targets}

~/Desktop/unitaryhack/Pulser/pulser/sequence.py in _delay(self, duration, channel)
    838         last = self._last(channel)
    839         ti = last.tf
--> 840         tf = ti + self._channels[channel].validate_duration(duration)
    841         self._add_to_schedule(channel,
    842                               _TimeSlot('delay', ti, tf, last.targets))

~/Desktop/unitaryhack/Pulser/pulser/channels.py in validate_duration(self, duration)
    103         if duration < self.min_duration:
    104             raise ValueError("duration has to be at least "
--> 105                              + f"{self.min_duration} ns.")
    106 
    107         if duration > self.max_duration:

ValueError: duration has to be at least 16 ns.

I don't understand how there can be any duration issue there.

[LaurentAjdnik]

Hi @Slimane33.

I'm investigating on this one and will let you know ASAP.

[LaurentAjdnik]

Let's test the case when x=45.

The second pulse on ch0 occurs from t=1824 to t=2004.

The previous pulse on ch1 occured from t=1772 to t=1824, which brings us to t=1992 with the 220ns retargeting time.

Target #0 being used by both channels, the next pulse on ch1 has to be delayed to t=2004, hence a need for a 12ns delay.

This is where the error happens, because 12ns is smaller than the minimal delay of 16ns.

@HGSilveri: I think we should not check against Channel.min_delay when we just want to add some delay. What do you think ?

Here's the sequence when disabling this check (we can notice the 12ns delay on ch1):

Channel: ch0
t: 0 | Initial targets: 0, 1, 2, 3, 4, 5, 6, 7, 8 | Phase Reference: 0.0
t: 0->180 | Pulse(Amp=10 rad/µs, Detuning=0 rad/µs, Phase=0) | Targets: 0, 1, 2, 3, 4, 5, 6, 7, 8
t: 180->1824 | Delay
t: 1824->2004 | Pulse(Amp=10 rad/µs, Detuning=0 rad/µs, Phase=0) | Targets: 0, 1, 2, 3, 4, 5, 6, 7, 8

Channel: ch1
t: 0 | Initial targets: 0 | Phase Reference: 0.0
t: 0->180 | Delay
t: 180->232 | Pulse(Amp=0 rad/µs, Detuning=10 rad/µs, Phase=0) | Targets: 0
t: 232->232 | Target: 1 | Phase Reference: 0.0
t: 232->284 | Pulse(Amp=0 rad/µs, Detuning=20 rad/µs, Phase=0) | Targets: 1
t: 284->452 | Target: 2 | Phase Reference: 0.0
t: 452->504 | Pulse(Amp=0 rad/µs, Detuning=30 rad/µs, Phase=0) | Targets: 2
t: 504->672 | Target: 3 | Phase Reference: 0.0
t: 672->724 | Pulse(Amp=0 rad/µs, Detuning=40 rad/µs, Phase=0) | Targets: 3
t: 724->892 | Target: 4 | Phase Reference: 0.0
t: 892->944 | Pulse(Amp=0 rad/µs, Detuning=50 rad/µs, Phase=0) | Targets: 4
t: 944->1112 | Target: 5 | Phase Reference: 0.0
t: 1112->1164 | Pulse(Amp=0 rad/µs, Detuning=60 rad/µs, Phase=0) | Targets: 5
t: 1164->1332 | Target: 6 | Phase Reference: 0.0
t: 1332->1384 | Pulse(Amp=0 rad/µs, Detuning=70 rad/µs, Phase=0) | Targets: 6
t: 1384->1552 | Target: 7 | Phase Reference: 0.0
t: 1552->1604 | Pulse(Amp=0 rad/µs, Detuning=80 rad/µs, Phase=0) | Targets: 7
t: 1604->1772 | Target: 8 | Phase Reference: 0.0
t: 1772->1824 | Pulse(Amp=0 rad/µs, Detuning=90 rad/µs, Phase=0) | Targets: 8
t: 1824->1992 | Target: 0 | Phase Reference: 0.0
t: 1992->2004 | Delay
t: 2004->2056 | Pulse(Amp=0 rad/µs, Detuning=10 rad/µs, Phase=0) | Targets: 0
t: 2056->2212 | Target: 1 | Phase Reference: 0.0
t: 2212->2264 | Pulse(Amp=0 rad/µs, Detuning=20 rad/µs, Phase=0) | Targets: 1
t: 2264->2432 | Target: 2 | Phase Reference: 0.0
t: 2432->2484 | Pulse(Amp=0 rad/µs, Detuning=30 rad/µs, Phase=0) | Targets: 2
t: 2484->2652 | Target: 3 | Phase Reference: 0.0
t: 2652->2704 | Pulse(Amp=0 rad/µs, Detuning=40 rad/µs, Phase=0) | Targets: 3
t: 2704->2872 | Target: 4 | Phase Reference: 0.0
t: 2872->2924 | Pulse(Amp=0 rad/µs, Detuning=50 rad/µs, Phase=0) | Targets: 4
t: 2924->3092 | Target: 5 | Phase Reference: 0.0
t: 3092->3144 | Pulse(Amp=0 rad/µs, Detuning=60 rad/µs, Phase=0) | Targets: 5
t: 3144->3312 | Target: 6 | Phase Reference: 0.0
t: 3312->3364 | Pulse(Amp=0 rad/µs, Detuning=70 rad/µs, Phase=0) | Targets: 6
t: 3364->3532 | Target: 7 | Phase Reference: 0.0
t: 3532->3584 | Pulse(Amp=0 rad/µs, Detuning=80 rad/µs, Phase=0) | Targets: 7
t: 3584->3752 | Target: 8 | Phase Reference: 0.0
t: 3752->3804 | Pulse(Amp=0 rad/µs, Detuning=90 rad/µs, Phase=0) | Targets: 8

@HGSilveri: I think there's another bug on ch1 since the first two pulses should be separated by the retargeting time, shouldn't they?

[HGSilveri]

@Slimane33 thanks for spotting this and thanks @LaurentAjdnik for the detailed report.

@HGSilveri: I think we should not check against Channel.min_delay when we just want to add some delay. What do you think ?

In fact, the delay that is actually programmed in hardware, so it has to obey that condition. Instead of disabling the check, we should use np.clip() to round it up to at least the chosen channel's min_duration before validating it.

@HGSilveri: I think there's another bug on ch1 since the first two pulses should be separated by the retargeting time, shouldn't they?

The retarget_time is actually the minimum time that has to occur between two target instructions on a channel. You don't have it on the first retarget because the last "target" instruction happened all the way back at t=0, so more than 220 ns have since elapsed.

[LaurentAjdnik]

In fact, the delay that is actually programmed in hardware, so it has to obey that condition. Instead of disabling the check, we should use np.clip() to round it up to at least the chosen channel's min_duration before validating it.

Thanks for you feedback, @HGSilveri.

A few more questions before I PR something.

Why np.clip()? duration being an int, there seems to be no array involved.

Plus, is there an upper bound we should check for? Or is checking against Channel.min_duration sufficient?

[HGSilveri]

Why np.clip()? duration being an int, there seems to be no array involved.

Just out of convenience, to avoid an if statement. It works for single values just as well.

Plus, is there an upper bound we should check for? Or is checking against Channel.min_duration sufficient?

No, no upper bound to worry about here. If you're using np.clip, set the upper bound to np.inf

[LaurentAjdnik]

Why np.clip()? duration being an int, there seems to be no array involved.

Just out of convenience, to avoid an if statement. It works for single values just as well.

Plus, is there an upper bound we should check for? Or is checking against Channel.min_duration sufficient?

No, no upper bound to worry about here. If you're using np.clip, set the upper bound to np.inf

I benchmarked (with timeit for 100 000 loops) three options:

Using if       : 0.00602359999999999
Using max()    : 0.012266000000000027
Using np.clip(): 1.0756528

The if statement was 180 times faster than np.clip().

Plus, I consider that using straightforward statements always makes code more readable and maintainable.

If you don't mind, I'll PR with this option.

[HGSilveri]

I don't mind of course, go ahead!

[LaurentAjdnik]

Hi @Slimane33. An update has been merged (see #205). Can you check again and let us know if it works now? Thanks!