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

Biasstep rebias #362

Merged
merged 31 commits into from
Sep 3, 2023
Merged

Biasstep rebias #362

merged 31 commits into from
Sep 3, 2023

Conversation

yuhanwyhan
Copy link
Contributor

this contains the script of re-biasing detectors using biasstep.
this function has recently been tested in LATR-t with different loading conditions.
this plot is showing the result of re-biasing detectors when LATR-t ful exposed to the room
Screenshot 2023-06-20 at 11 58 53 AM
this plot is showing the result of re-biasing detectors after covering the LATR-t with a metal plate
Screenshot 2023-06-20 at 11 59 40 AM
Notice the tests were done with UHF detectors (Uv31), and the change of loading between these two test are very large.
in the field we expect lower and more gradually change of loadings.

The next level thing is to make the script run faster, this might involve only running part of the analysis code for bias-step.

Copy link
Collaborator

@dpdutcher dpdutcher left a comment

Choose a reason for hiding this comment

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

From what I understand, the functionality is good, so my comments are more code hygiene things. There's some excess whitespace, long lines, and repeated sections of code that should be cleaned up. Doing so will help with readability and make debugging easier if needed.

sodetlib/operations/bias_dets.py Outdated Show resolved Hide resolved
Scripts work well when detectors are already in transistion. For extreme cases
such as detectors are all in SC, it takes a few run to put them perfectly in
transition. Next step is making this script can handle extreme cases faster

Copy link
Collaborator

Choose a reason for hiding this comment

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

Docstring should have purpose of the function and description of args/kwarrgs. This note is fine to put after those items.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also an overview of how the script works would be nice.

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

sodetlib/operations/bias_dets.py Outdated Show resolved Hide resolved
sodetlib/operations/bias_dets.py Show resolved Hide resolved
time.sleep(60)
safe_dc_biases = previous_dc_biases.copy()
for replace_bg in bg_overbias_needed:
safe_dc_biases[replace_bg] = cfg.dev.bias_groups[replace_bg]["testbed_100mK_bias_voltage"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that a standard entry in the config file? In other words, is it guaranteed to exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not a standard entry but should be added in.
the purpose for this entry is so the script has a reference of what bias_voltage to use such that the detectors won't be in SC state at the site. This can also be understood as the bias_voltages at the minimum loading.
since all modules are tested in Princeton first, the dark detector bias_voltages are naturally the bias_voltages at the minimum loading. If this info is missing for some reason, user can put in some high numbers such as 15V for Uv31 as a start point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you do some combination of (a) adding that parameter to det_config.py (b) using a .get() call here to return a sensible default if no matching entry is found?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I second this. If you add it to the bg_defaults it will always be loaded and you won't need the get call though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a way in the current det_config.py such that we can load different numbers for 90s and 150s?

Copy link
Member

Choose a reason for hiding this comment

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

Could we name this something other than testbed bias voltage? I saw "testbed" and was very confused about why that was useful on-site until I saw you explanation here.

"dark_bias_voltage" or even "reference_bias_voltage" could make this clearer. And it would (correctly) imply that we could change these numbers later as we start to know the UFMs and on-sky loading better.

sodetlib/operations/bias_dets.py Outdated Show resolved Hide resolved
Comment on lines +286 to +297
## add in check if detectors are normal now
percentage_rn_0 = bsa_0.R0/bsa_0.R_n_IV
drop_from_normal = False
for bl in bg_overbias_needed:
mask_bg = np.where(bsa_0.bgmap==bl)[0]
per_bl_percentage_rn_0 = percentage_rn_0[mask_bg]
## mask for normal
mask_normal = np.where((per_bl_percentage_rn_0 > 0.9) | (per_bl_percentage_rn_0 < -0.9))
## if more than half of the detectors are normal
if len(mask_normal[0]) > 0.5*len(mask_bg):
bg_detectors_normal.append(bl)
drop_from_normal = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code block is repeated from before (I think). Could be tidied up to avoid repetition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code block is not repeated. this code block is designed to be ran again if over_biasing is needed. After the overbiasing, the script has to choose some bias_voltage to land on, in order to be safe from dropping to SC again, the script will choose the biasvoltage recorded in the config file as "testbed_100mK_bias_voltage", this however might land the detectors in normal state. Hence this code block is for catching this after the overbiasing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I'm trying to point out is that

mask_bg = np.where(bsa_0.bgmap==bl)[0]
per_bl_percentage_rn_0 = percentage_rn_0[mask_bg]
## mask for normal
mask_normal = np.where((per_bl_percentage_rn_0 > 0.9) | (per_bl_percentage_rn_0 < -0.9))
## if more than half of the detectors are normal
if len(mask_normal[0]) > 0.5*len(mask_bg):
bg_detectors_normal.append(bl)
drop_from_normal = True
are the exact same lines as
mask_bg = np.where(bsa_0.bgmap==bl)[0]
per_bl_percentage_rn_0 = percentage_rn_0[mask_bg]
## mask for normal
mask_normal = np.where((per_bl_percentage_rn_0 > 0.9) | (per_bl_percentage_rn_0 < -0.9))
## if more than half of the detectors are normal
if len(mask_normal[0]) > 0.5*len(mask_bg):
bg_detectors_normal.append(bl)
drop_from_normal = True

so it could be, for example, condensed into a helper function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah yeah good point... but I am hoping it is ok to just leave it like this as it just used only twice

sodetlib/operations/bias_dets.py Show resolved Hide resolved
sodetlib/operations/bias_steps.py Show resolved Hide resolved
@jlashner
Copy link
Collaborator

jlashner commented Jun 20, 2023

Hi Yuhan, I'm about to start reviewing this, but do you think you can post here (or in the docstring) like the overall strategy for how this works? Something like a flow-chart that documents what it will try to do at each step under what conditions, etc. would be really helpful for understanding this at a higher level

@yuhanwyhan
Copy link
Contributor Author

Hi Yuhan, I'm about to start reviewing this, but do you think you can post here (or in the docstring) like the overall strategy for how this works? Something like a flow-chart that documents what it will try to do at each step under what conditions, etc. would be really helpful for understanding this at a higher level

the main logic is described in 4.2.2 of
https://arxiv.org/pdf/2208.05997.pdf

extra parts of this script is for handling failures of bias_step and handling extreme situations of detectors stucked in SC state or in normal state

Comment on lines 261 to 269
if np.isnan(np.nanmedian(per_bl_R0)):
## the yield is very low, could it be the biasstep analysis script failed? repeat again
repeat_biasstep = True
S.log("the biasstep script might have failed, going to repeat the measurement")

if repeat_biasstep:
S.log("retaking biasstep due to failure")
bsa_0 = bias_steps.take_bias_steps(S, cfg)
repeat_biasstep = False
Copy link
Collaborator

@jlashner jlashner Jun 20, 2023

Choose a reason for hiding this comment

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

I don't really like that this is forcing a retake of the bias steps, especially since if it fails a second time it will just continue on as if everything's ok. I think instead if the first bias steps fails, this should raise an exception with a useful error message so a user can take control and figure out the issue.

Do you know how often this fails / why? We should try to fix in the bias step function directly instead of building fixes into this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bias step failure has been noticed in multiple places including at the site in LATR, in Chicago LATRt, TSAT and Cornell. One of the failure mode might due to the if statement here:

if not transition:

Tho repeating the biasstep is not ideal, it is a working method given the current circumstances.

Copy link
Collaborator

@jlashner jlashner Jun 20, 2023

Choose a reason for hiding this comment

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

I don't see how repeating it changes anything though... If we want, we can pass an arg that forces it to use the transition estimation of resistance if we think it should be in the transition state. also I believe Yaqiong is also working on a better way to determine whether a channel is in transition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but simply repeating it worked. yes Yaqiong and I have been reviewing some functions together.In my opinion I think the if statement can just be removed as we don't really rely on a accurate tes resistance from the biasstep when TES is not in-transition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it's mostly failing when a large fraction of detectors are superconducting? I notice you don't have retries for other times you call bias steps.

Copy link
Contributor Author

@yuhanwyhan yuhanwyhan Jun 20, 2023

Choose a reason for hiding this comment

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

I added in a retry for each bias step except the last one I think. I think the failing can happen either when a large fraction of detectors are superconducting or being normal.
also repeating biasstep twice is like a old voodoo from my ACT days. I think such fix might make sense if one imagines some ADC needs to be 'waken'. I am not sure but I agree this can be redundant and can be removed after better understanding later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-06-20 at 6 57 13 PM Screenshot 2023-06-20 at 6 57 38 PM this is one of the failure case I just found after taking another bias_step this is fixed this is taken in LATRt, on Uv31, I believe the session id is 1687298049

Copy link
Contributor Author

@yuhanwyhan yuhanwyhan Jun 20, 2023

Choose a reason for hiding this comment

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

failure:
Screenshot 2023-06-20 at 7 02 57 PM

Screenshot 2023-06-20 at 7 03 11 PM Screenshot 2023-06-20 at 7 03 22 PM

after repeating
Screenshot 2023-06-20 at 7 01 39 PM
Screenshot 2023-06-20 at 7 01 52 PM
Screenshot 2023-06-20 at 7 02 01 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome, that's really interesting. are these files all on simons1? I'll take a look tomorrow.

Comment on lines 305 to 306
for sleep_index in range(5):
time.sleep(60)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the sleep-time after overbiasing should be a configurable parameter, ideally settable in the dev-cfg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good idea.

Comment on lines +311 to +315
except KeyError:
mask_bg = np.where(bsa_0.bgmap==replace_bg)[0]
v_norm_bl = np.nanmedian(iva.v_bias[iva.idxs[[mask_bg], 1]])
safe_dc_biases[replace_bg] = v_norm_bl

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need this except anymore since it's in the default dev cfg dict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree.
speaking of this, what should be the correct way of generating a config file. I am not sure generating from the default dev cfg dict is practiced a lot currently in different places

Copy link
Collaborator

Choose a reason for hiding this comment

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

In your sysconfig, if you set the devcfg path as one that doesn't exist, it will try to create a new one there with the defaults

Comment on lines 341 to 380
while drop_from_normal:
S.log(f"some biaslines are still in normal state: {bg_detectors_normal}")
percentage_rn_0 = bsa_0.R0/bsa_0.R_n_IV
S.log("droping detectors from normal to transition")
previous_dc_biases_dfn = S.get_tes_bias_bipolar_array()

S.log(f"current tes bias voltages are: {previous_dc_biases_dfn}")
for bl in bg_detectors_normal:
mask_bg = np.where(bsa_0.bgmap==bl)[0]
v_sc = iva.v_bias[iva.idxs[[mask_bg], 0]]
v_norm = iva.v_bias[iva.idxs[[mask_bg], 1]]
v_spread = np.nanmedian(v_norm) - np.nanmedian(v_sc)
if previous_dc_biases_dfn[bl] > v_spread:
previous_dc_biases_dfn[bl] = previous_dc_biases_dfn[bl] - 0.5 * v_spread
else:
previous_dc_biases_dfn[bl] = previous_dc_biases_dfn[bl] - 0.3 * v_spread
bg_detectors_normal.remove(bl)
S.log(f'biasline {bl} is approching 0 voltage, a confirmation new IV curve is recommended')

S.set_tes_bias_bipolar_array(previous_dc_biases_dfn)
S.log(f"applying: {previous_dc_biases_dfn}")
S.log(f"waiting 10s for dissipation of UFM local heating")
time.sleep(10)
bsa_0 = bias_steps.take_bias_steps(S, cfg)
percentage_rn_0 = bsa_0.R0/bsa_0.R_n_IV
drop_from_normal = False
for bl in bg_detectors_normal:
mask_bg = np.where(bsa_0.bgmap==bl)[0]
per_bl_percentage_rn_0 = percentage_rn_0[mask_bg]
## mask for normal
mask_normal = np.where((per_bl_percentage_rn_0 > 0.9) | (per_bl_percentage_rn_0 < -0.9))
## if more than half of the detectors are normal
if len(mask_normal[0]) > 0.5*len(mask_bg):
drop_from_normal = True
else:
bg_detectors_normal.remove(bl)
for bl in bg_detectors_normal:
if previous_dc_biases_dfn[bl] < 0.5:
bg_detectors_normal.remove(bl)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain a bit about what this is doing? I don't quite understand how you're chosing the bias voltage step sizes, and also it's not clear to me from the code how many times this runs on average, but seems like it could be a lot of bias step measurements. Do you think this is mainly what's dominating the function's runtime?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we might want to brainstorm other ways to drop into the transition. I think if we're taking small enough step sizes we may not need to take bias steps every time, since we're unlikely to induce tracking skips.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is looping down the bias voltages until a certain bias line is no longer in normal state and TES start to be in-transtion. The step size being the 50% of the transition region from the latest IV such that the bias_line won't be dropped into SC due to step being too large or the script takes too long due to the step being to small. Notice the latest IV doesn't necessary provides good estimation as the I-V won't be taken as frequently as the bias-step, so a larger step size here can be dangerous.
in the field I think the function runtime is dominated by the bias-step analysis itself. This is because in the field the sky loading changes gradually and slowly. the longest runtime will be the wait time of biasing detectors out of SC state, but this shouldn't happen if the re-biasing is done frequently enough nor should dropping from normal state happen frequently.

S.log(f"applying: {previous_dc_biases_dfn}")
S.log(f"waiting 10s for dissipation of UFM local heating")
time.sleep(10)
bsa_0 = bias_steps.take_bias_steps(S, cfg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you do not need to run on all bias lines, you can specify the bgs arg here to just run on a selection of them. This may speed up the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a good idea, but I think this should be added into the next generation of this code. This is because I currently rely on bg map for all 12 biaslines in this version of code.

@jlashner
Copy link
Collaborator

Hi Yuhan, finally was able to read through the entire function. Here are the notes I took about the control flow. Do you agree with this as the intended behavior?

  1. Take initial bias steps bsa_0
    • will rebias if there are any bias lines where R0 is all nan
    • If >10% of dets are have Rfrac < 0.1, marks bg as overbias_needed
    • If >50% of dets have Rfrac > 0.9, marks bg as normal (and drop from normal needed)
  2. Overbias overbias_needed bgs
    • runs overbias_dets on bgs that need overbiasing, and waits
    • For biaslines that have been overbiased, set the DC bias voltage to testbed_100mK_bias_voltage
    • Run bias steps
      • (no bias retry here)
    • Check normal condition (above) and add updated bias lines to bg_detectors_normal array if they meet it
  3. Drop from normal
    • For each normal bg, step voltage by 0.5(V_norm - V_SC) determined from IV
    • Run bias steps
      • update bsa_0 so it's always the last bias step ran
      • These bias steps don't have a retry
    • Check normal condition. If any bgs are still normal, repeat.
    • After exiting loop, the resulting DC biases are initial_dc_biases
  4. Find new bjas voltage and take bias steps:
    • vspread is diff between median normal and median SC voltages based on IV
    • New bias voltage is init +/- 0.15*vspread where +/- is determined by
      whether or not the median Rfrac is > or < than 0.5
    • Take bias steps (bsa_1) and retry if needed
  5. Determine new bias point for each channel:
    • v_estimate = (v0 (target - Rfrac_1) + v1 (Rfrac_0 - target)) / (Rfrac_0 - Rfrac_1)
    • vnew = median(v_estimate[bg]), or if all are nan, vnew = (v1 - v0) / 2
  6. Set bias to vnew and retake bias steps bsa_2 (with retry if needed)
    • BG is successful if abs(Rfrac - target) < 0.05
  7. Fine tuning unsuccessful bias groups
    • Step 0.15 vspread in the appropriate direction
    • Take bias steps (with retry)
    • v_estimate2 = (v0 (target - Rfrac_1) + v1 (Rfrac_0 - target)) / (Rfrac_0 - Rfrac_1)
      • with v0= prev estimate and v1 = prev estimate + delta
    • Apply v_estimate2, retake bias steps.
  8. Return final bias step and bias voltages

@jlashner
Copy link
Collaborator

Can you give an estimate of the run-time?

@yuhanwyhan
Copy link
Contributor Author

Can you give an estimate of the run-time?

4 mins if start from in-transition, 14 mins if start from SC. (tested in LATRt)

@msilvafe
Copy link
Contributor

I like this description of the flow of the function. Since @yuhanwyhan gave the thumbs up to the description can you commit that to the docs rst file along with a link to Yuhan's conference proceedings @jlashner ?

@yuhanwyhan
Copy link
Contributor Author

there is one potential rare failure mode I just realized.
let's assume a situation that there is one bad biasline that causes heating or that biasline is short to a FR, and we don't want to touch it at all. The current script will still take a few bias_steps on this biasline. Although if a biasline is bad, we will not put bias_voltage on it to start with, so taking a biasstep round 0 voltage shouldn't do much damage, still....

the solution for this issue will require some re-writing.

the current script forces taking bg map for all 12 biaslines because when it calculates the v_estimate, it does the calculation for all biaslines and then apply the bg_map. One possible fix will be adding a for loop looping over the chosen biaslines and apply the bg_map inside the for loop such that the script only calculate the v_estimate for the chosen biaslines.

I suggest adding this fix into the later version of this script when we can test it cold again. Right now without a cold cryostat and MF/UHF UFMs, I am not confident that this change won't break the code.

@yuhanwyhan
Copy link
Contributor Author

a way to make the script faster will be only running the R_tes calculation from biasstep except the final biasstep (running only the _compute_R0_I0_Pj part). An estimation of the run-time for each analysis block of the current biasstep function will be nice.

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.

Since this has been tested, I'm fine with merging this now with minimal changes to the code. I think we can merge as long as we make the following updates to the documentation:

  • Add a link to the spie proceeding describing the procedure in the docstring
  • Add my step-by-step overview of what the function is doing to the docstring (or a better overview if someone else wants to write one)
  • Add a comment or some sort of documentation describing how testbed_100mK_bias_voltage is being used. I'm not exactly sure how this is used right now, but think we may want to change the name a bit, since this will be used in non-testbed systems, and I think this should probably have the word "transition" in it somewhere. Maybe 100mK_transition_bias_voltage?

As we test this more and start to use it in the field, I think we will probably want to clean up / update the functionality, but that can all go into separate PRs.

@@ -201,3 +201,408 @@ def bias_to_rfrac(S, cfg, rfrac=0.5, bias_groups=None, iva=None,

return biases

def biasstep_rebias(
Copy link
Member

Choose a reason for hiding this comment

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

Could we set this function as an action? We will want this to be categorized as a single detector operation instead of "many many" bias steps in the data packaging. We'll also want to change the default tags getting added to these .g3 files.

@jlashner jlashner self-requested a review September 2, 2023 19:32
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.

Added a simple docs page linking to the spie paper, and made the function a pysmurf action. I think I am happy to merge this in now and solve any problems with it as they come up.

@msilvafe
Copy link
Contributor

msilvafe commented Sep 2, 2023

Added a simple docs page linking to the spie paper, and made the function a pysmurf action. I think I am happy to merge this in now and solve any problems with it as they come up.

@kmharrington mentioned 2 additional things needed for this to be used within the site data operations, and packaging:

  1. Getting the .g3 tags in the file to be something other than oper,bias_steps so they aren't all made into different operations books.
  2. Figure out how to get imprinter to bind them all together into one book

I’m happy to wait on 2 until later but if 1 is a simple-ish modification I would like to get that in before merge. We want to use this soon in SAT-MF1 though so if not simple ok to merge and open new issue/or with these additional things.

@jlashner
Copy link
Collaborator

jlashner commented Sep 2, 2023

Right, thanks for reminding me. Latest commit should do that, and sets all g3_tags to oper,biasstep_rebias

@msilvafe
Copy link
Contributor

msilvafe commented Sep 2, 2023

Right, thanks for reminding me. Latest commit should do that, and sets all g3_tags to oper,biasstep_rebias

Thanks let's get this merged then

@jlashner jlashner merged commit 64ccde4 into master Sep 3, 2023
1 check 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.

5 participants