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

PLE scan #132

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

PLE scan #132

wants to merge 33 commits into from

Conversation

LaneUW
Copy link
Collaborator

@LaneUW LaneUW commented Dec 19, 2023

Adds functionality to perform a PLE scan.
With the GUI launched by PLEscan.py, you can set your voltage sweeps and read out the results from the SPCM

Copy link
Collaborator

@gadamc gadamc left a comment

Choose a reason for hiding this comment

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

Hi Lane - thanks for all of this work. I hope it's functioning well in the lab soon.

There are some broad changes to your PR I think we should make. Overall, since this code is now becoming used beyond the qt3 lab, we should adopt some better engineering practices that I have ignored (which was a huge mistake on my part).

  1. I've installed flake8 in my IDE and it complains about various spacing around = and , characters. These complaints exist through a lot of my original code (which you copied) and we should strive to fix all of this (not now, but at some point in time either incrementally or in one big code cleanup). So, if you see a lot of complaints from flake8/black or other linter that's based on PEP8, please fix those instead of copying my code verbatim.
  2. The src/applications folder is deprecated. We should move PLEscan.py to src/qt3utils/applications/qt3ple/main.py. Perhaps ask the team though if another name besides qt3ple is warranted.
  3. Add qt3ple (or other name) to pyproject.toml
  4. Type hinting -- please add this throughout all of your changes. This will really help future dev work by lab members. I've added type hints all throughout the latest version of the qt3scope and qt3scan applications.
  5. python modules should be all lower case(https://peps.python.org/pep-0008/#package-and-module-names). PLEscanner.py -> ple_scanner.py

src/applications/PLEscan.py Outdated Show resolved Hide resolved
src/qt3utils/nidaq/customcontrollers.py Outdated Show resolved Hide resolved
src/qt3utils/nidaq/customcontrollers.py Outdated Show resolved Hide resolved
src/qt3utils/nidaq/customcontrollers.py Outdated Show resolved Hide resolved
src/qt3utils/nidaq/customcontrollers.py Show resolved Hide resolved
@LaneUW LaneUW requested a review from gadamc January 8, 2024 23:18
Copy link
Collaborator

@vasilisniaouris vasilisniaouris left a comment

Choose a reason for hiding this comment

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

Overall, this looks good. Check my review for explicit feedback on specific code sections. I still need to go through part of the code, but these comments can get us started.

src/qt3utils/nidaq/customcontrollers.py Show resolved Hide resolved
src/qt3utils/nidaq/customcontrollers.py Outdated Show resolved Hide resolved
src/qt3utils/nidaq/customcontrollers.py Outdated Show resolved Hide resolved
src/qt3utils/nidaq/customcontrollers.py Outdated Show resolved Hide resolved
src/qt3utils/nidaq/customcontrollers.py Outdated Show resolved Hide resolved
src/qt3utils/datagenerators/plescanner.py Outdated Show resolved Hide resolved
self.scanned_count_rate = []


class WavemeterAndScanner(PleScanner):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall, this is a good implementation. if people perform multiple scans, it might be a good idea to implement an attribute so that users can choose between unidirectional or bidirectional scans. In the first case, the start and stop values are the same, but in the second case they flip for each consecutive scan (and the step size changes sign).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be easy to add. Jotting this down more as a note to myself: I should talk to Nick again about the sweeps while I'm thinking about this. There was something about the time of the downsweep for the unidirectional case which he said was a concern. I believe it had to do with hysteresis. I'll check so I can knock both of these out at once.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check in with Nick for the specific implementation on their setup, but for general use, I know that other people would be happy to have the bidirectional scan as an option.

src/qt3utils/datagenerators/plescanner.py Outdated Show resolved Hide resolved
src/qt3utils/datagenerators/plescanner.py Outdated Show resolved Hide resolved
src/qt3utils/applications/qt3ple/main.py Outdated Show resolved Hide resolved
src/qt3utils/applications/qt3ple/main.py Outdated Show resolved Hide resolved
src/qt3utils/applications/qt3ple/main.py Outdated Show resolved Hide resolved
src/qt3utils/applications/qt3ple/main.py Outdated Show resolved Hide resolved
src/qt3utils/applications/qt3ple/main.py Outdated Show resolved Hide resolved
src/qt3utils/applications/qt3ple/main.py Show resolved Hide resolved
@vasilisniaouris vasilisniaouris self-requested a review March 5, 2024 23:25
src/qt3utils/applications/qt3ple/main.py Outdated Show resolved Hide resolved
src/qt3utils/applications/qt3ple/main.py Outdated Show resolved Hide resolved
src/qt3utils/applications/qt3ple/main.py Outdated Show resolved Hide resolved
src/qt3utils/applications/qt3ple/main.py Outdated Show resolved Hide resolved


class MainApplicationView():
def __init__(self, main_frame, scan_range=[0, 2]) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

How difficult would it be to change the default values on the GUI from the yaml? This idea should be a separate issue if we think we should go forward with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The GUI is loaded before a YAML file is read, so difficult. But it should be easy to have it change the values once the YAML file is read. I think I can cook that up real fast.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that sounds good to me!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, you need to implement a gui for the user to update the configuration through.

src/qt3utils/nidaq/customcontrollers.py Show resolved Hide resolved
src/qt3utils/nidaq/customcontrollers.py Outdated Show resolved Hide resolved
src/qt3utils/datagenerators/plescanner.py Outdated Show resolved Hide resolved
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.

4 participants