wiregrid: Add bias step measurements during calibration

[sadachi5]

Purpose

To understand the time constant deeply, we would like to perform bias step measurements before and after the wiregrid calibration.

Changes

• Add bias step measurements before and after the insert() and also eject() in calibrate() function.
• Add exception for insert() in the calibrate() function, and add eject() in the exception.

Changed files

• src/sorunlib/wiregrid.py
• tests/test_wiregrid.py

[ykyohei]

Thanks for making this PR, I would definitely like to have biasstep when wiregrid is in! I'm not sure how to solve codecov error, we may need Brian's advice.

1. I'm fine but I wonder having biasstep 4 times might be too much, we might only need twice.
2. I'm not sure how helpful it is to take data when wiregrid is moving (insert/eject). I think it's more helpful to stream for 30 seconds when wiregrid is ejected.

[BrianJKoopman]

I'm not sure how to solve codecov error, we may need Brian's advice.

If you click through on the failing check, '...' -> 'View details' -> 'View this Pull Request on Codecov', you'll see a page like this: https://app.codecov.io/gh/simonsobs/sorunlib/pull/239?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=simonsobs

There you'll see lines not covered by tests. The decrease in test coverage is what's causing the check to fail. So the test cases do not cover, in this instance, the lines where an insert fails and we try to eject.

I'm wondering about the utility of ejecting after a failed insert. How likely is it that this works? Everywhere else we just fail when an insert/eject happens and state that the position needs to be inspected before continuing observations. I sort of think we should do the same here.

[sadachi5]

Thanks for making this PR, I would definitely like to have biasstep when wiregrid is in! I'm not sure how to solve codecov error, we may need Brian's advice.

1. I'm fine but I wonder having biasstep 4 times might be too much, we might only need twice.

I agree with you. I modified the argument. Now, we have bias_step_wo_wg(defualt=True) and bias_step_wt_wg(default=False). Nominally, we just perform the bias_step before insert and after eject (without wiregrid on the window). But, for the test or future request, I added the argument bias_step_wt_wg, which will perform the bias_step before and after the rotation with wiregrid on the window.

2. I'm not sure how helpful it is to take data when wiregrid is moving (insert/eject). I think it's more helpful to stream for 30 seconds when wiregrid is ejected.

For example, what is the purpose of the 30-second stream? If needed, I can add it.

[sadachi5]

I'm not sure how to solve codecov error, we may need Brian's advice.

If you click through on the failing check, '...' -> 'View details' -> 'View this Pull Request on Codecov', you'll see a page like this: https://app.codecov.io/gh/simonsobs/sorunlib/pull/239?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=simonsobs

There you'll see lines not covered by tests. The decrease in test coverage is what's causing the check to fail. So the test cases do not cover, in this instance, the lines where an insert fails and we try to eject.

I fixed it.

I'm wondering about the utility of ejecting after a failed insert. How likely is it that this works? Everywhere else we just fail when an insert/eject happens and state that the position needs to be inspected before continuing observations. I sort of think we should do the same here.

That’s a good point. We had one insert failure before because the limit switch didn’t work properly. It only happened once, so I agree that it’s better to remove the exception and ensure that we inspect the position before continuing observations.

[ykyohei]

I'm not sure how helpful it is to take data when wiregrid is moving (insert/eject). I think it's more helpful to stream for 30 seconds when wiregrid is ejected.

For example, what is the purpose of the 30-second stream? If needed, I can add it.

The purpose of 30 seconds stream with wiregrid ejected is to measure the non-wg instrumental polarization (background polarization). Currently wg-pileline estimates background pol as component which does not depend on wg angle from data when wiregrid is inserted, but I think direct measurement can be helpful to measure polarization efficiency. I thought this was also the reason why "calibrate" start to stream before wg insertion. I think the data when wiregrid is moving is much more complex and is not very useful.

[ykyohei]

1. I'm fine but I wonder having biasstep 4 times might be too much, we might only need twice.

I agree with you. I modified the argument. Now, we have bias_step_wo_wg(defualt=True) and bias_step_wt_wg(default=False). Nominally, we just perform the bias_step before insert and after eject (without wiregrid on the window). But, for the test or future request, I added the argument bias_step_wt_wg, which will perform the bias_step before and after the rotation with wiregrid on the window.

I should have commented more explicitly.
I think bias_step_wt_wg is more important for wiregrid calibration so it should be run by default, bias_step_wo_wg is helpful to understand instruments but it's less useful for calibration. I was actually thinking in different way. I thought running pair of biasstep with and without wiregrid before "calibration" might be enough. because the duration of wg measurement is short and drift of detector properties are likely to be small. What do you think?

[sadachi5]

1. I'm fine but I wonder having biasstep 4 times might be too much, we might only need twice.

I agree with you. I modified the argument. Now, we have bias_step_wo_wg(defualt=True) and bias_step_wt_wg(default=False). Nominally, we just perform the bias_step before insert and after eject (without wiregrid on the window). But, for the test or future request, I added the argument bias_step_wt_wg, which will perform the bias_step before and after the rotation with wiregrid on the window.

I should have commented more explicitly. I think bias_step_wt_wg is more important for wiregrid calibration so it should be run by default, bias_step_wo_wg is helpful to understand instruments but it's less useful for calibration. I was actually thinking in different way. I thought running pair of biasstep with and without wiregrid before "calibration" might be enough. because the duration of wg measurement is short and drift of detector properties are likely to be small. What do you think?

Oh, I misunderstood. I agree that we prefer to have two bias steps with and without wg before calibration. For the time drift, the stepwise rotation itself takes ~4min. Probably, there is not much change. I changed the arguments to bias_step_before and bias_step_after, and set before as True and after as False for testing, just in case we want to do that.

[sadachi5]

@ykyohei
For the detector stream, I added a 10-second stream after ejecting to measure pol. eff. from the data with and without wiregrid. I thought 30 seconds was too long. I prefer 10 sec, which is the same duration as one angle step in the stepwise calibration.

[ykyohei]

Thank you for the changes, looks mostly good to me. I will leave Brian to approve this.

[sadachi5]

@BrianJKoopman
I and Kyohei approved the current scripts. Please check and approve it to merge.

[BrianJKoopman]

Thanks for the PR! I mostly have small comments about the error handling, and a question about the sleep after each bias step.

The large comment here relates to the structure of the calibrate function. Rather than having two separate blocks that deal with when we use a bias step flag vs when we do not, I suggested merging the two blocks, interrupting streaming where needed to run a bias step. Let me know whether you find this more readable or not.

[sadachi5]

@BrianJKoopman , @ykyohei
I added comments on Brian's comments. Please check them.

[BrianJKoopman]

@BrianJKoopman, @ykyohei I added comments on Brian's comments. Please check them.

Sorry, I'm not seeing responses or a new commit. Did your push not go through?

[sadachi5]

@BrianJKoopman, @ykyohei I added comments on Brian's comments. Please check them.

Sorry, I'm not seeing responses or a new commit. Did your push not go through?

I did not push anything. I commented on each of your comments before. Please check them.

[BrianJKoopman]

Thanks for the updates on this! Looks good to me.