Separate draw_samples and draw_sequence #533
Conversation
- transform inital_targets to a property - implements draw_samples and integrate it on draw_sequenece
- fix ChannelSamples field name and order
|
Hey @dakk, I am about to start reviewing your latest changes. |
| ) | ||
| else: | ||
| area_val = seq_.type.amplitude.integral / np.pi | ||
| phase_val = seq_.type.phase |
There was a problem hiding this comment.
[draw_phase_area] Phase is constant if not modulated, I propose you take phase_val as the phase at tf-1 (otherwise I guess it means that we definitely have to provide all the slots of ChannelSchedule to ChannelSamples as TimeSlots)
There was a problem hiding this comment.
does this seems a reasonable result? I used sampled_seq.channel_samples[ch].phase[slot.ti:slot.tf][-1]
There was a problem hiding this comment.
From the perspective of the phase area, it is great !
It is also working well from the phase value perspective, but I am noticing a difference between yours and the current version, your version is displaying all the phase whereas we are displaying only the phase shifts.
Nit: You can try sampled_seq.channel_samples[ch].phase[slot.tf-1], it should work as well (and be simpler)
There was a problem hiding this comment.
By using phase_val = sampled_seq.channel_samples[ch].phase[slot.tf-1] I was getting outofbound; I fixed by passing shown_duration as extended_duration to sample().
The other issue (displaying all the phase) was a wrong condition, I think I've solved it in the next commit.
There was a problem hiding this comment.
That's weird, to me sampled_seq.channel_samples[ch].phase[slot.ti:slot.tf][-1] should be equal to sampled_seq.channel_samples[ch].phase[slot.tf-1] - and it's weird that one raises an error without the other 😅
There was a problem hiding this comment.
They are not equal in python; [A:B] get a slice from index A to index B OR len() if B > len() (try [1,2][0:1000])
There was a problem hiding this comment.
Sure, thanks for reminding me ;)
| ) | ||
| else: | ||
| area_val = seq_.type.amplitude.integral / np.pi | ||
| phase_val = seq_.type.phase |
There was a problem hiding this comment.
From the perspective of the phase area, it is great !
It is also working well from the phase value perspective, but I am noticing a difference between yours and the current version, your version is displaying all the phase whereas we are displaying only the phase shifts.
Nit: You can try sampled_seq.channel_samples[ch].phase[slot.tf-1], it should work as well (and be simpler)
| va="center", | ||
| bbox=area_ph_box, | ||
| ) | ||
|
|
||
| # Draw target regions phase_shifts | ||
| if draw_phase_shifts: |
There was a problem hiding this comment.
In the end, why don't you pass _basis_ref to SequenceSamples when you sample a Sequence (just like _measurement and _magnetic_field) ?
Then you will be able to move this part of code in draw_channel_content.
There was a problem hiding this comment.
I did; if I didn't miss anything should be ok now; it is still missing coverage, but I wait for your approval first
There was a problem hiding this comment.
By the way, the coverage issues raises on EOM related parts: 68, 84-95, 211-213, 216-218, 230.
If before this pull request it was working, I think that using ChannelSamples.target_time_slots is not enough for handling this part.
There was a problem hiding this comment.
I am noticing an issue in the tutorial Output Modulation and EOM. The buffers before and after the EOM mode is switched on (the time slots that are just before and just after the eom interval) are not represented...
Obtained:
Should be:
There was a problem hiding this comment.
I see; I think the problem is in the gather_data part where we create eom buffers starting from ChannelSamples.target_time_slots; the drawing part is almost unchanged
There was a problem hiding this comment.
I further investigate it, but I cannot figure out what is the problem.
EDIT:
Just fixed
|
With the last commit I think I solved the EOM drawing; the coverage is almost complete, it's missing two lines: pulser-core/pulser/sampler/samples.py 183 1 99% 188 |
There was a problem hiding this comment.
All nice !
@dakk could you perhaps post some output of draw_samples for samples of the sequences defined in the Output Modulation and EOM Mode notebook and in (the tutorial about Phase Shifts)[https://pulser.readthedocs.io/en/stable/tutorials/phase_shifts_vz_gates.html]. It could be nice to verify that providing a register renders nicely, (for example with an SLM)[https://pulser.readthedocs.io/en/stable/tutorials/slm_mask.html].
@HGSilveri do you want to have a look ?
Core concepts and ideas subject to discussion:
draw_sequenceanddraw_sequenceboth callsdraw_channel_content(draw_samples can take a register to draw a register, and only shows the input sample - it is not possible to draw a modulated sample from a sample)- ChannelSamples has a list of TimeSlots of type "target", and can compute
is_eom_modeandget_eom_mode_intervals. - SequenceSamples has a new attribute _basis_ref that stores seq._basis_ref. There is surely a way to circumvent this but I don't know if it's worth spending time on it.
- the value of the phase shown with
draw_areais the last of the slot (works with non-modulated sample as phase is constant, should work with modulated samples as well as only beginning of the phase might be impacted).
Sure, these are images generated from the new branch; I'm comparing them with the stable branch, if there are differences I'm posting both.
|
|
I'm taking a look right now. A few preliminary comments:
Do you mean
I don't think so either, it's fine to include it imo.
Sounds ok, though I would probably go with the middle instead (ie @ (ti + tf)/2) |
|
@HGSilveri I indeed mean |
| # If slot is not the first element in schedule | ||
| if sch.in_eom_mode(slot): | ||
| if ch_samples.in_eom_mode(slot): | ||
| # EOM mode starts | ||
| if not in_eom_mode: | ||
| in_eom_mode = True |
There was a problem hiding this comment.
So indeed I would first try with np.isclose(extended_samples.amp[slot.ti], 0). However, if the sample is the result of a modulated sample then it is not going to be close to 0 at slot.ti. I think it's best to check that np.isclose(extended_samples.amp[slot.tf-1], 0) or (slot.ti + slot.tf)/2.
There was a problem hiding this comment.
Neither one is working; the missing smooth_draw area in eom_end_buffers is from 940 to 1180, which was this delay _TimeSlot in the original schedule _TimeSlot(type='delay', ti=940, tf=1180, targets={0, 1, 2, 3}) (Still using EOM Mode Operation https://pulser.readthedocs.io/en/stable/tutorials/output_mod_eom.html as test case)
So the check on amp value should be correct, but it is not including the delay slot.
The output is restored as the original if I do this; if the current slot ti is different from the current eom_block end (tf), then it means there is a delay and we can add an end_buffer for this missing interval:
if slot.ti != eom_intervals[eom_block_n].tf:
if extended_samples.amp[eom_intervals[eom_block_n].tf] == 0:
eom_end_buffers[eom_block_n] = EOMSegment(
eom_intervals[eom_block_n].tf, slot.ti
)
elif extended_samples.amp[slot.ti] == 0:
eom_end_buffers[eom_block_n] = EOMSegment(
slot.ti, slot.tf
)I'm not sure if the elif part is still necessary and if this version is semantically identical to the original.
And this is the result:
There was a problem hiding this comment.
We are indeed trying to assert if there is a delay there, so I think your proposal has the intended effect. The elif should be dropped
There was a problem hiding this comment.
Good job finding this ! My question was related to using draw_samples on a modulated sample: imagine the user provides the hashed green curve to display (draw_samples(sampler.sample(seq, modulation=True))), how do you display the EOM mode and its buffer ? I think the first buffer will be shown, the main EOM part as well, but that you will have to check that your amplitude is not equal to 0 right after tf but after the fall time of the EOM (2 * cast(BaseEOM, extended_samples._ch_objs[ch].eom_config).rise_time, see in ChannelSamples.modulate())
if slot.ti != eom_intervals[eom_block_n].tf:
if extended_samples.amp[eom_intervals[eom_block_n].tf + 2 * cast(BaseEOM, extended_samples._ch_objs[ch].eom_config).rise_time] == 0:
eom_end_buffers[eom_block_n] = EOMSegment(
eom_intervals[eom_block_n].tf, slot.ti
)
There was a problem hiding this comment.
You were right, the ending part was not show when drawing with draw_samples(sampler.sample(seq, modulation=True)): your solution works for the ending part.
The problem remains with the eom_start_buffers; the condition extended_samples.det[slot.tf - 1] == ch_samples.eom_blocks[eom_block_n + 1].detuning_off never becomes True, but the values are close:
-0.49731363802743167 instead of -0.5; can I solve this by using np.isclose with a relative factor of 0.005, but I want your opinion for that
There was a problem hiding this comment.
I think that using np.isclose and defining a rtol is definitely the way to go. Eventually I would make it dependent on the modulation_bandwidth (I think the higher the modulation bandwidth, the higher the error will be, so I guess you can define the relative tolerance as an empirical factor times the modulation bandwidth)
There was a problem hiding this comment.
Ok, I'll try making some experiment on it
There was a problem hiding this comment.
Doesn't the mod_bandwidth change only the rising time? I don't understand how it can affect the amplitude or the detuning
| ch_data = data[ch] | ||
| ch_obj = sampled_seq._ch_objs[ch] | ||
| ch_eom_intervals = data[ch].eom_intervals | ||
| ch_eom_start_buffers = data[ch].eom_start_buffers | ||
| ch_eom_end_buffers = data[ch].eom_end_buffers |
There was a problem hiding this comment.
line 416 can you check that draw_output is false in the EOM tutorial for draw_samples ?
There was a problem hiding this comment.
Yes, if I use draw_samples for the EOM tutoral, draw_output is False
There was a problem hiding this comment.
Yes of course. Sorry I just thought you were displaying curves obtained with draw_samples and not draw_sequence. I can access the curves you showed in the readthedocs, but I would like to see what is shown if you replace the seq.draw() instruction in these notebooks by:
sampled_seq = sampler.sample(seq)
draw_samples(sampled_seq)
mod_sampled_seq = sampler.sample(seq, modulation = True)
draw_samples(mod_sampled_seq)
There was a problem hiding this comment.
Images are: first is draw_samples(sampler.sample(seq)) and second is draw_samples(sampler.sample(seq, modulation = True)); I'm also passing the parameters passed to the draws in the notebook (ie: draw_phase_shifts)
Phase shifts and vz:
Output Modulation
EOM mode (using the isclose with the relative factor I shown before)
SLM Mask:
There was a problem hiding this comment.
Thanks for having a look at this, looks good to me !
This is a bit concerning to me actually, how come it was working before but not now? There seems to be something slightly different about the |
- rename _TargetSlot to _PulseTargetSlot - fix duration for draw_samples - fix eom_buffers creation - fix draw_phase_area
| ch_samples.slots[-1].tf | ||
| for ch_samples in sampled_seq.channel_samples.values() | ||
| ] | ||
| max_slot_tf = max(slot_tfs) if len(slot_tfs) > 0 else None |
There was a problem hiding this comment.
This is introduced since if the total_duration is less that the last slot.tf, it raise an out of bound while accessing .amp, .phase and .det
- create those buffer in Schedule.get_samples - Adapt _seq_drawer gather_data for the new buffers - add a test assert for ChannelSamples.in_eom_mode
|
@dakk The failing CI tests seem to be due to the latest release of numpy (1.25.0). We'll look into it on our end, but you can pin the numpy version in the requirements to |
HGSilveri
changed the title
Sure, thank you too (and @a-corni ofc) for the fundamental great support you gave me! @a-corni unitaryhack deadline is in ~20 hours (11.59 PM anywhere on earth), so iff everything is fine also for you please merge before it! |
Fix #522