c_norm & sc_off tracking, optional base_dir to allow load if differen…

[rsonka]

Added saving (during analyze_iv()) of c_norm (phase offset constant) & sc_off (superconducting offset) to IVAnalysis (and documented them). Set _load_am() to automatically initialize those arrays in the class if they didn't. (I'm assuming I don't need to do anything special for axis manager with those?).

Added an optional argument base_dir to _load_am() to allow loading of IVAnalysis save files if sodetlib is run on a different timestream directory structure, so that I could test my code on Princeton's dedicated analysis computer.

I also did a few minor aesthetic improvements by lining up = signs with spaces.

[jlashner]

Hi Rita, thanks for this! Looks mostly good, but I have a few small comments inline.

Also I appreciate the formatting changes, however technically it's against pep8 formatting rules and I think we should change it back. I agree that in some cases it does look better, but this kind of thing is difficult to maintain, since you'll have to change the alignment for a bunch of lines whenever a longer variable is added.

[jlashner]

I like the addition of these fields, however the names aren't so intuitive to me. Do you think we can rename them to something like normal_offset and sc_offset, and maybe add something like "y-intercept of the normal/superconducting branches in the raw data", since I'm not sure if that's clear just from the term "offset".

Also do you think we can specify units here? You say "phase" but I think the units might be amps.

[rsonka]

First of all, thanks for the detailed and useful review!

Also I appreciate the formatting changes, however technically it's against pep8 formatting rules and I think we should change it back.

Okay, will do. I didn't know we were following pep8 formatting rules (I've mostly been following the style guide I was taught in college; I now wonder why they didn't tell me about pep8, seeing as it predated that class.). Perhaps add that to the Github main page so future contributors aren't surprised by it?

this kind of thing is difficult to maintain, since you'll have to change the alignment for a bunch of lines whenever a longer variable is added.

My python instructor might have taught us this way because he was a fan of vim/emacs, which makes that much easier to change.

Do you think we can rename them to something like normal_offset and sc_offset, and maybe add something like "y-intercept of the normal/superconducting branches in the raw data", since I'm not sure if that's clear just from the term "offset".

c_norm stands for "constant of normal branch fit", as opposed to s_norm, "slope of normal branch fit." I agree that your names are improvements individually (and am a bit embarrassed I didn't make better names myself.). However given that sc_offset is only an offset of the superconducting branch, I'm not keen on using "normal_offset" for an offset applied to the entire response (though it is nice in implying where it comes from). "i_tes_offset" might be better in that vein (especially paired with your proposed i_tes vs. resp idea below)?

Also do you think we can specify units here? You say "phase" but I think the units might be amps.

I'm not a SQUID readout expert, but by my understanding: the original Resp is a measure of difference in the recorded phase from when you start taking data, and the phase offset correction (the normal branch offset) is the final step in converting them to amps (In fact, it's common for resp to be very negative before that correction is performed, which is unphysical given how we measure). The fact that we're measuring in phase difference is why we have to do the phase offset correction in the first place. However, it's true that we do it after we do this:

A_per_rad = iva.meta['pA_per_phi0'] / (2*np.pi) * 1e-12
iva.resp = (iva.resp.T * iva.polarity).T * A_per_rad

...and I support doing it in that location. So...I see what you mean in that it's closer to being Amps than it is to being phase. I guess it would be better to call it "[proto-Amps]" or something (similarly for resp in your proposed i_tes vs. resp idea below, which I approve of).

[rsonka]

(I'm going to peruse PEP8, so it might be a bit before I actually implement these changes and respond to the rest of your review.).

[rsonka]

EDIT: I decided o work on this before finishing pep8. I've performed several updates to the original comment after reviewing things more.

[jlashner]

I was thinking a bit about this and realized that resp and i_tes are currently exactly the same. This may have been what you were suggesting before, but if you want we can have it so resp does not have the offsets subtracted out, while i_tes does. Up to you if you want to make those changes here.

Anyway, I think this docstring is definitely correct, but kind of technical, like it's hard to understand just reading it without knowing what the code is doing. Do you think we can change so it's a bit less technical? Maybe something like

offsets are subtracted from the full IV and the superconducting branch so that the superconducting 
and normal branches both have a y-intercept of 0.

[jlashner]

I think it might make a bit more sense to put this in the load function instead of _load_am

[msilvafe]

Hi @rsonka, @jlashner and I are trying to do a new year's cleaning up of open PRs in sodetlib. Would you be able to give us an idea of when you could make the requested changes so we can merge this PR? Let us know if we can help.

For PEP8 formatting you can use a linter (such as pylint) in whatever IDE you use (I use one in vim and vscode), it'll check the pep8 format and tell you how to fix it. While reading the PEP8 manual is not a bad idea a linter is probably an easier path to getting the formatting changes here fixed. Thanks again for these contributions!