Auto determination of center frequencies.#584
Conversation
* Fixes bug when using multiple frequencies for one slice, i.e. `freq_order` defined for the slice without `rxctrfreq` or `txctrfreq` * No more rxctrfreq, txctrfreq in experiment slices * Uses all frequencies in an AveragingPeriod to determine center frequencies * Moved center frequency determination code into its own file in utils/frequencies.py (out of utils/interface_classes/averaging_periods.py) * Added experiment tests for auto-tuning * Added unit tests for new frequencies.py file
There was a problem hiding this comment.
The code looks good and is sufficiently understandable. Logic seems clear. The test suite seems to hit all the bases from what I can tell (I have a couple more tests to suggest at the end actually). I approve with some minor documentation added.
I would add a few extra comments here and there for maximal clarity. First, in frequencies.py, under determine_tuning_freqs, the algorithm processes freqs in list order and doesn't backtrack when forming groups. I understand this is intentional to preserve the frequency/range order. But someone seeing this for the first time may be confused and not understand the why something like [10001, 13500, 11750] would not just trivially group 10001 and 11750 together and may try to correct the "error" without fully understanding the behaviour first. So a brief comment explaining the rationale there could be good. Something outlining units in the docstring could also be helpful? A source on where the reference for the 50kHz no-go zone (and the no-go zone for the edges) comes from could be helpful as well, just for the sake of hardware documentation.
Maybe in averaging_periods.py under _determine_tuning_freqs a brief note can be added to the docstring explaining why SEQUENCE or CONCURRENT need the same tuning frequencies while freq_order slice wouldn't. It is not immediately obvious.
Lastly, a test could be added to determine_tuning_freqs where it has a mixed list of plain frequencies and Band ranges together. Currently they are only tested separately. What about a single element freqs test as well?
| usable due to distortions around the tuning frequency and from the intrinsic N200 anti-aliasing filter. As such, | ||
| determination of tuning frequencies is somewhat complex as there are effectively 5 regions to consider: | ||
|
|
||
| |xxxxxx------------xxxxx------------xxxxxx| |
|
Not passing actions, but I'll make a new PR for that to keep the history clean. |
3 participants
freq_orderdefined for the slice withoutrxctrfreqortxctrfreq