Skip to content

Conversation

@jlashner
Copy link
Contributor

@jlashner jlashner commented Mar 7, 2023

Adds tag param to pysmurf-controller operations, which will append a user-provided tag to the end of the g3 stream tag.

This + simonsobs/sodetlib#336 resolves #385

Motivation and Context

Helpful for the scheduler to be able to append tags to streams or other sodetlib operations.

How Has This Been Tested?

Not tested yet. This should be tested with simonsobs/sodetlib#336 before merging.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

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.

This looks good to me, just two small comments on the docstrings.

I'll defer to @kmharrington about whether this satisfies #385.

@BrianJKoopman BrianJKoopman added the enhancement New feature or request label Mar 8, 2023
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.

Changes look good! Just one additional suggestion from me.

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.

I have a new question about this. It looks like g3_tag on operations overrides default tags like "oper,bgmap". (Should we always be overriding these?)

I just want to confirm that adding a user defined tag on a stream doesn't override the default "obs,stream" tags.

@jlashner
Copy link
Contributor Author

Right, in conjunction with this sodetlib PR, user-defined tags are set through a separate kwarg from the stream type/subtype. So when we test we'll need to test both these PRs together.

Copy link
Member

@kmharrington kmharrington left a comment

Choose a reason for hiding this comment

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

Reviewing in combo with this one: simonsobs/sodetlib#336

Looks good to me modulo the one in-line comment. I was able to test passing subtypes to observations with stream with and w/o extra tags. I was also able to test extra tags while taking IVs and noise. The bias steps and bgmaps are throwing errors but I'm told that's due to a different error that has been fixed elsewhere. So assuming the tag implementation is the same between all the operations we should be good here!

@BrianJKoopman
Copy link
Member

Great, thanks for testing @kmharrington!

@jlashner can we merge simonsobs/sodetlib#336 and then also bump the docker base image for the controller in this PR?

@BrianJKoopman BrianJKoopman merged commit f4e6c20 into main Jun 5, 2023
@BrianJKoopman BrianJKoopman deleted the pysmurf_controller_tags branch June 5, 2023 22:05
hnakata-JP pushed a commit that referenced this pull request Apr 12, 2024
* adds pysmurf-controller tag params

* Update test

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fix docs and small changes

* Fix tests

* Adds choices to stream_type

* bump sodetlib

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pysmurf-controller needs to deal with tags in more concrete way.

4 participants