Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Biasstep rebias #362
Changes from 17 commits
bc1a609
a3a1176
f87db7a
3e3eefc
d27467b
14bbfd6
42c66fd
dde5a01
2dd275d
043eea5
4122222
858bcec
e480145
db0b073
d06ad93
77df01c
ce45d67
56ee133
aefe183
05fea5d
87fbf18
e651ce4
92e29a8
ef1f259
2d3b2fb
076db68
6dd0517
01fd395
1327af0
b2b6101
7e25878
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
sodetlib/sodetlib/operations/bias_steps.py
Line 645 in 4aafb94
Tho repeating the biasstep is not ideal, it is a working method given the current circumstances.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
failure:
after repeating
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good idea.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 theget
call though.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
sodetlib/sodetlib/operations/bias_dets.py
Lines 325 to 332 in 05fea5d
sodetlib/sodetlib/operations/bias_dets.py
Lines 289 to 296 in 05fea5d
so it could be, for example, condensed into a helper function
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.