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

gui,scan: add CenterScan Scannable variant #1193

Merged
merged 1 commit into from Nov 15, 2018
Merged

Conversation

jordens
Copy link
Member

@jordens jordens commented Nov 14, 2018

  • parametrized by center/span/step instead of
    start/stop/npoints which is more convenient in some applications
  • no scan widget support so far

Signed-off-by: Robert Jördens rj@quartiq.de

ARTIQ Pull Request

Description of Changes

Type of Changes

Type
✨ New feature

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

  • Use correct spelling and grammar.
  • Update RELEASE_NOTES.md if there are noteworthy changes, especially if there are changes to existing APIs.
  • Close/update issues.
  • Check the copyright situation of your changes and sign off your patches (git commit --signoff, see copyright).

Code Changes

  • Run flake8 to check code style (follow PEP-8 style). flake8 has issues with parsing Migen/gateware code, ignore as necessary.
  • Test your changes or have someone test them. Mention what was tested and how.
  • Add and check docstrings and comments
  • Check, test, and update the conda recipes in conda/
  • Check, test, and update the unittests in /artiq/test/ or gateware simulations in /artiq/gateware/test

Documentation Changes

  • Check, test, and update the documentation in doc/. Build documentation (cd doc/manual/; make html) to ensure no errors.

Git Logistics

  • Split your contribution into logically separate changes (git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.
  • Write short & meaningful commit messages. Review each commit for messages (git show). Format:
    topic: description. < 50 characters total.
    
    Longer description. < 70 characters per line
    

Licensing

See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.

* parametrized by center/span/step instead of
  start/stop/npoints which is more convenient in some applications
* no scan widget support so far

Signed-off-by: Robert Jördens <rj@quartiq.de>
@jordens jordens added this to the 4.0 milestone Nov 14, 2018
@jordens
Copy link
Member Author

jordens commented Nov 14, 2018

Since this will break the (major version-ed) dashboard UI state format I'd like to get this into 4.0.

@sbourdeauducq
Copy link
Member

Looks good.

@sbourdeauducq sbourdeauducq merged commit 494ffca into master Nov 15, 2018
@dhslichter
Copy link
Contributor

@jordens a couple of thoughts:

  1. I think center/span/npoints is a more common use case than center/span/step, although certainly both could be used. My sentiment arises from the notion that in an experiment one often wants to scan over a span centered around a given frequency, where you either change the span or the center, but in changing the span one (typically) keeps the number of points the same, such that it takes the same amount of time to run the scan. I think the case of widening the span while keeping the step/resolution constant is less common than widening the span while keeping number of points constant. Therefore I would suggest it's more suitable to parameterize by number of points because then one needn't change both span and step when changing span and desiring to keep run time constant.

  2. It appear that the current code doesn't scan in order -- sequence goes from center + step to center + int(span/(2.*step))*step, then back to center and scanning out to center - int(span/(2.*step))*step. Am I reading this incorrectly? I'd suggest it should go from center - int(span/(2.*step))*step to center + int(span/(2.*step))*step, passing through center in the middle.

@jordens
Copy link
Member Author

jordens commented Nov 15, 2018

There are multiple ways to parametrize a scan. start, stop and center, span are a conjugate. As are span, step and span, npoints. If you are in a situation where you are searching for a feature that has a known size (e.g. fourier limited) and is located in a certain span, the parametrization implemented here is the suitable and convenient one. Another use case is when you are generating "nice" publication data where you are interested in an even step size. Also generally when the number of points is irrelevant.
List comprehension in Python has (at least to me) somewhat counterintuitive precedence. It scans center first and then alternates the deviation from the center with increasing distance from center.

@jordens jordens deleted the centerscan branch November 15, 2018 22:17
@dhslichter
Copy link
Contributor

@jordens ack. Thanks for clearing up the Python list comprehension. Also, my preference for npoints over step is a weak one; I usually just do the math in my head anyway with the current scan type. It feels too much like feature creep in the GUI to have all four kinds of scans (start,stop with npoints or step, and center, span with npoints or step) separately listed with radio buttons, but could one make a clean GUI interface for toggling between step and npoints within a given RangeScan or CenterScan? I might have a think about this if you are not opposed. If you are strongly against it, forget I mentioned it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants