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

Add bias wave task to pysmurf-controller agent #633

Merged
merged 9 commits into from
Mar 28, 2024

Conversation

bkelle
Copy link
Contributor

@bkelle bkelle commented Feb 23, 2024

adding in code for bias waves. Essentially copying the bias step code block and modifying for bias wave analysis.

Description

adding in agent task for bias waves in similar form to bias step

Motivation and Context

Needed to be able to run bias waves in nextline

bkelle and others added 2 commits February 23, 2024 13:51
adding in code for bias waves. Essentially copying the bias step code block and modifying for bias wave analysis.
Copy link
Collaborator

@jlashner jlashner left a comment

Choose a reason for hiding this comment

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

Looks good to me! a few inline comments. Also need to register the OCS operation at the bottom of the file.

I'd like this to be tested before merging, so we should work out when to do that.

socs/agents/pysmurf_controller/agent.py Show resolved Hide resolved
Comment on lines 840 to 844
session data object. See the `sodetlib bias wave docs page

NEED TO CREATE DOCS PAGE AND INSERT HERE

for more information on bias wave and what kwargs can be passed in.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets leave this out until the docs page exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in latest commit

bkelle and others added 3 commits February 23, 2024 14:36
added in param for "tag" arg for bias wave (and biasstep... seemed to be missing there too). Rmoved docstring for nonexistent documentation.
registered OCS operation at bottom of file
@bkelle
Copy link
Contributor Author

bkelle commented Feb 23, 2024

Looks good to me! a few inline comments. Also need to register the OCS operation at the bottom of the file.

I'd like this to be tested before merging, so we should work out when to do that.

I've made the requested changes, and registered the OCS operation at the end of the file! This should be good to test, I will interface with the site team to see when a good time for me to do that will be.

Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

Looks good to me, there was a stray decorator added to another task that should be removed (see below.)

Also, this bias wave code isn't merged upstream in sodetlib yet, so those tests are going to keep failing, and the build isn't going to work. That should be merged first.

socs/agents/pysmurf_controller/agent.py Outdated Show resolved Hide resolved
@BrianJKoopman BrianJKoopman changed the title Update agent.py for bias waves Add bias wave task to pysmurf-controller agent Feb 23, 2024
@msilvafe
Copy link
Contributor

Looks good to me, there was a stray decorator added to another task that should be removed (see below.)

Also, this bias wave code isn't merged upstream in sodetlib yet, so those tests are going to keep failing, and the build isn't going to work. That should be merged first.

This was merged in sodetlib here can we rerun ci tests somehow?

@BrianJKoopman
Copy link
Member

Merging in the latest main would do it.

@msilvafe
Copy link
Contributor

@BrianJKoopman this test failure doesn’t seem related to anything we added.

@BrianJKoopman
Copy link
Member

@BrianJKoopman this test failure doesn’t seem related to anything we added.

Yup, agreed. Not sure why this is cropping back up suddenly, but #643 should work around the issue here.

@BrianJKoopman
Copy link
Member

Alright, #650 dropped the unrelated tests that are being flaky. Merge in the latest main when you're ready.

@BrianJKoopman BrianJKoopman mentioned this pull request Mar 27, 2024
6 tasks
@BrianJKoopman
Copy link
Member

Sorry, one more merge of the latest main. #652 drops the now failing tests.

Copy link
Contributor

@msilvafe msilvafe left a comment

Choose a reason for hiding this comment

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

Thanks, let's get this merged and running on the telescopes :)

@jlashner jlashner self-requested a review March 28, 2024 13:08
@jlashner jlashner merged commit 2af3236 into simonsobs:main Mar 28, 2024
5 checks passed
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