Bias Wave #412
Bias Wave #412
Conversation
|
Ok, I think this is ready to test. It currently can take data at multiple frequencies but only runs the analysis on the lowest frequency to calculate the DC parameters. We can add in analysis functions later for calculating time constants with multiple frequencies (there's some inline comments with placeholders for those things to go). This should probably come with an accompanying PR in socs to add an agent task but that can come after testing. I'm going to convert from draft to full PR so you all can start reviewing but I we will wait until we have taken some test data on SATP1 or SATP3 before merging. |
|
Spent some time looking through this this morning. I think it looks good and I agree its ready to test as soon as we have a telescope we can try to run it on. That being said i'm not a total expert when it comes to sodetlib code writing. But I think this captures what Yuhan and I were doing in notebooks. Do I need to approve this commit before being able to test it, or should we test it and then approve after that? |
Please test on this branch, I'm not confident that the data taking won't just fail because I have a missed colon at this point...(I mostly don't think that's true bc I did run a linter on this but I think you get my point). So checkout this branch on the server and test the data taking and analysis fn's and confirm that everything looks good. Post whatever plots/etc are useful from your testing here in the review to show that you've tested that it works and that can serve as your approval to merge. |
|
Sounds good Max. We will do that. Hopefully we can work it in to the SATp3 schedule when observing again, I think things have been down for a few days? In any case I'll interface with Yuhan, Sam, and folks at the site to test in this branch. thanks! |
Ok, sounds good! Let me know when you get a chance to take a stab at it. I can try to be available to help debug any issues that arise. |
There was a problem hiding this comment.
Thanks for this! I'm really glad it works, and though there is still work to be done testing it and making sure it is correct, I'm not opposed to merging this with a few docs requests:
- Can we write a nice docstring for the BiasWaveAnalysis class? In particular, something that documents the saved results, their shapes, etc.
- Can we try to remove as much vistigial "bias-step" code and docs as possible? There are a few things in here that are copied over from the bias step module (both in code and in docs), which make it difficult to know what is actually important for this analysis vs what is just copied over. I think if we can do a once over and try to remove as much of that as possible that would be great
Also, I question whether the analysis, as written, will function correctly, but fixing that can wait til a proper study is done and put into a second PR.
sodetlib/operations/bias_wave.py
Outdated
|
|
||
| class BiasWaveAnalysis: | ||
| """ | ||
| UPDATE THE DOCSTRING...Maybe we can ask Ben to do this one. |
There was a problem hiding this comment.
I think this should be written, along with some sort of documentation before merging.
There was a problem hiding this comment.
Hi Jack, I'll work on this! Yeah, was planning to fill this out before merging.
sodetlib/operations/bias_wave.py
Outdated
|
|
||
| self.start_idxs = np.full(np.shape(self.start_times), np.nan) |
There was a problem hiding this comment.
I think this is basically the line that caused all the problems we later had to fix yesterday @msilvafe . Do we want to do something else here, or leave it as is knowing that we modified things elsewhere to accommodate this?
There was a problem hiding this comment.
Ya I think probably we want to change this to ints and then get rid of all of the hacky things we did later on to convert the values from this array to ints (and fix any checks we have for nans to not be based off of this array).
There was a problem hiding this comment.
There's no nan equivalent int- would it be better to assign 0's (or maybe some arbitrarily high number) and then have an if statement so it doesn't analyze those values?
There was a problem hiding this comment.
well start_times is a float. How about you just use that everywhere instead of start_idxs? Oh actually I'm already using start_times for the np.isnan() check so let's just initialize start_idxs as ints filled with -1
There was a problem hiding this comment.
actually I can just put an if statement in so it fills amplitudes for empty slice BL. is it better to fill with 0 or np.nan amplitudes?
There was a problem hiding this comment.
I'm not sure what you mean @bkelle/are concerned by I think amplitudes are already filled with nans right? (I think nan are preferable).
There was a problem hiding this comment.
Oh I was saying that idk how the get_amplitudes works on a zero-length slice. so I can write an "if statement" to only calculate amplitudes for non-zero length slices, and else assign amplitudes with np.nan
There was a problem hiding this comment.
Is there a way that you think that could happen?
There was a problem hiding this comment.
actually I think this is fine, testing the periodogram func it doesnt break if you feed it an empty array it just spits out nothing. so I'm not gonna change anything for now beyond just changing the .astype(int) fixes.
|
|
||
| bwa.sid = sdl.stream_g3_on( | ||
| S, tag=g3_tag, channel_mask=channel_mask, downsample_factor=1, | ||
| filter_disable=True, subtype=stream_subtype, enable_compression=enable_compression | ||
| ) | ||
|
|
There was a problem hiding this comment.
Out of curiosity... how long does a typical stream take and how large are the g3 files? Bias steps, which takes ~1 min are fairly large, so if this takes much longer then that may potentially be an issue.
There was a problem hiding this comment.
running each frequency on each BL for 3 seconds
There was a problem hiding this comment.
Depends on duration, and number of freqs arguments. Maybe @bkelle can give you the answer on the 2-frequencies and 3 sec duration test run we did and could probably do some scaling tests (for both time to run and data size) if we can get more time on satp3.
There was a problem hiding this comment.
It looks like yesterday for 2 frequencies it took about 2 mins! Which is basically consistent with bias steps then, ~1 min per frequency. I guess that means we should think about the data collection paradigm for i.e. time constant analysis with multiple frequencies as this could get large with like 7 frequencies or something like that.
Fixed a few of the requested changes from Jack: Deleted unused __init__ removed some unused arg docstrings changed start_idxs and stop_idxs assignment to be ints
whoops misunderstood Jack's comment on the __init__ modification. Fixing it to align with what he meant here!
adding in sign convention for phase on dIrat calculation. Negative sign = in transition. This is a response to Jack's earlier comments!
slight modification to previous commit in the phase sign calculation code.
included code to write chunked bias signal to bwa object. Also removed some phase calc code that probably wont work, in advance of testing said code separately.
sodetlib/operations/bias_wave.py
Outdated
| @@ -292,6 +292,7 @@ def _get_wave_response(self, am=None): | |||
| npts = np.nanmin(self.stop_idxs-self.start_idxs) | |||
|
|
|||
| sigs = np.full((nchans, n_freqs, npts), np.nan) | |||
| biases = np.full((nchans, n_freqs, npts), np.nan) | |||
There was a problem hiding this comment.
This should not be nchans x nfreqs x npts it should be nbgs x npts
I have tested ~I think~ all parts of these updates in a notebook. Using new get amplitude method of deprojection for recovering amplitude and phase data. I didn't yet remove the old get_amplitudes function, it would be trivia to delete it if that's better practice.
There was a problem hiding this comment.
Thanks for the doc updates, I'm fine with merging this for now.
I'm still not 100% confident in the DC param estimation and phase determination. In particular, like max said, there are better ways to figure out whether the signals are in phase that we should think about...
Before using in operation, I'd like to see a study comparing directly DC params computed with bias waves with those from bias steps in and out of transition. Does that exist right now? If so can we link the presentation / confluence page? Thanks!
This is a draft PR for now...still lots to go but can start code level discussion here to move some of it off of slack.
The intention of this PR is to implement a new "bias wave" module that calculates equivalent parameters as the bias_step operation but using small sine waves on the bias line which can be processed using fourier analysis which is a bit cleaner in the presence of large halfwave plate synchronous signal.