Skip to content

Conversation

@tachikoma-li
Copy link
Member

This function should be the same as the one in core with some tweaks. We expect users to use it to generate amplitude noise characterization pulses.
Tests are migrated and modified a bit.

@tachikoma-li tachikoma-li requested review from a team, charmasaur and leoadec August 25, 2020 23:18
@leoadec
Copy link
Member

leoadec commented Aug 26, 2020

I think I might be missing some context here about why we're transferring this to Open Controls?

@tachikoma-li
Copy link
Member Author

I think I might be missing some context here about why we're transferring this to Open Controls?

User guide is using old API to generate noise characterization pulses, which we want to get rid of.
To fully migrate to the new API, we want the pulse generation can be don with Open Control (like in other guides), which would save us from writing a new API for it.

},
)

segment_start_times = np.arange(0, duration, minimum_segment_duration)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that it's a specified duration but only a minimum segment duration, I think instead we should use slightly longer segment lengths so that we get a pulse of duration duration exactly:

segment_count = int(np.ceil(duration / minimum_segment_duration))
segment_start_times = np.arange(segment_count) * duration / segment_count # or similar
...

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, sorry I missed that point. Fixed.

base_gaussian_segments
)
maximum_rotation_angle = (
minimum_segment_duration
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make the suggested change above, this would become segment_duration

Copy link
Member

@leoadec leoadec left a comment

Choose a reason for hiding this comment

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

The code looks good, I just have two suggestions.

I don't think I have the authority to oversee the transfer of code from a private repo to an open source project, so I'll let @charmasaur give the final thumbs up here

)

# work out exact segment duration
segment_num = int(np.ceil(duration / minimum_segment_duration))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe segment_count?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, somehow I always pick segment_num

Comment on lines 894 to 896
base_gaussian_segments = (1.0 / gaussian_width / np.sqrt(2 * np.pi)) * np.exp(
-0.5 * ((segment_midpoints - gaussian_mean) / gaussian_width) ** 2
)
Copy link
Member

Choose a reason for hiding this comment

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

My opinion of this back when it was part of Core is that the normalization constant 1.0 / gaussian_width / np.sqrt(2 * np.pi) isn't necessary because the pulse gets normalized again anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

SG, removed.

@tachikoma-li tachikoma-li merged commit c4827e0 into master Aug 26, 2020
@tachikoma-li tachikoma-li deleted the MG branch August 26, 2020 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants