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

Moving run_tuning into scripts directory #18

Merged
merged 8 commits into from
May 6, 2020
Merged

Conversation

jlashner
Copy link
Collaborator

@jlashner jlashner commented Apr 20, 2020

This PR is to start the process to move Rita's run_tuning script into the main scripts directory. Here are a few minor changes I made to rita's script:

  1. Adds detconfig object to handle the configuration (i.e. drive power / some other pysmurf arguments)
  2. Removes most of the argparse / function arguments. I think we should approach scripts by starting with a minimal set of arguments and adding more either with argparse or through the device config file as they become necessary.
  3. Pysmurf uses a single tune file for all bands, so I removed the tunefile dictionary and replaced it with the single tunefile.

The way this is written now, the find_and_tune_freq function will update the config object, so if we run a bunch of functions in serial you can access the correct tuning info with cfg.dev.exp['tunefile'], but the device config file isn't actually overwritten. That's done at the end of the script. We should discuss if this is what we actually want, and keep behavior consistent between sodetlib funcitons.

I have not tested it yet, so we should do that on the UCSD system before merging.
Rita and Max, can you look this over and let me know if there's anything you think should change? For instance if you think we should be allowing for more pysmurf arguments to be settable we should put those into the device config files before merging.

Resolves #6.

@jlashner jlashner requested review from rsonka and msilvafe April 20, 2020 23:24
@rsonka
Copy link
Contributor

rsonka commented Apr 23, 2020

These are major enough changes that the script really needs retesting.
In particular, I'm not sure it will actually write a tuning file without the ability to set new_master_assignment=True....tracing it, at a glance it looks like it doesn't.

I know we also used the subband argument to limit to specific areas to save time. It doesn't currently look like that can be changed via config or command line.

I personally want the ability to set plotname_append from the command line (speaking as the person who got it into pysmurf). This script isn't necessarily something that gets run once at the start of data taking, and that's it. If you're making changes in between testing runs in a single session (ex. bath temperature), you can end up rerunning it multiple times, and it's very convenient for on-the-spot checks to append a note on which run it was to all the plots from this script, so you can find them and contrast them easily with your eyes or with eog when you later get a moment while you're waiting for a script or temperature change.

I also notice that the drive/drive_power naming issue still hasn't been resolved.

Further comments as I get a bit more time, especially to scan the config files and understand more of how that works.

@rsonka
Copy link
Contributor

rsonka commented Apr 23, 2020

Looking at the config file usage in my script, I'd think that plotname_append should definitely be a command line argument, new_master_assignment should probably be a command line argument, and subband should be in the config file.

@jlashner jlashner force-pushed the merge_run_tuning branch from ac27992 to 651bcdc Compare May 4, 2020 23:27
…nd is ready to be merged into master. Added in statements to catch if no resonances/empty tunefile is output and to pass setup_notches if find_freq doesn't find anything. Removed plotname_append argparse since we will reserve this functionality for interactive python sessions.
@msilvafe msilvafe merged commit 50e918e into master May 6, 2020
@msilvafe msilvafe deleted the merge_run_tuning branch May 6, 2020 16:11
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.

Tuning script
3 participants