PowerSpectrum: improve diagnostic value power spectrum api #695
Conversation
JoepVanlier
force-pushed
the
lazy_power
branch
11 times, most recently
from
ba1a1a5 to
aea2040
Compare
JoepVanlier
force-pushed
the
lazy_power
branch
from
aea2040 to
7aec2c8
Compare
JoepVanlier
changed the title
JoepVanlier
requested review from
alessiamarcolini,
rpauszek and
hrishikeshkelkar
JoepVanlier
force-pushed
the
lazy_power
branch
from
7aec2c8 to
e3f38c4
Compare
JoepVanlier
force-pushed
the
lazy_power
branch
from
e3f38c4 to
867865e
Compare
JoepVanlier
force-pushed
the
lazy_power
branch
from
867865e to
8cc0ad8
Compare
JoepVanlier
force-pushed
the
expose_diode_model
branch
from
c7dd04e to
a11ff69
Compare
JoepVanlier
force-pushed
the
lazy_power
branch
from
8cc0ad8 to
1ee98bd
Compare
JoepVanlier
force-pushed
the
lazy_power
branch
from
1ee98bd to
c37465a
Compare
JoepVanlier
force-pushed
the
lazy_power
branch
from
c37465a to
f725db9
Compare
JoepVanlier
force-pushed
the
lazy_power
branch
2 times, most recently
from
d07cc94 to
1c92eaf
Compare
There was a problem hiding this comment.
Looks good, although I'm not gonna pretend that I got to deep into the intricacies of the math that was going on but looks mostly like rearranging and as always thoroughly tested. Just some minor nitpicks, mostly some extra comments would be good for those of us that don't know force calibration like the back of our hand
changelog.md
Outdated
| * Allow showing the ranges that were excluded from a power spectrum by passing `show_excluded=True` to [`PowerSpectrum.plot()`](https://lumicks-pylake.readthedocs.io/en/latest/_api/lumicks.pylake.force_calibration.power_spectrum.PowerSpectrum.html#lumicks.pylake.force_calibration.power_spectrum.PowerSpectrum.plot). | ||
| * Allow plotting the ranges that were excluded from fitting and the active calibration peak when plotting a calibration result using `show_excluded=True` and `show_active_peak=True` in [`CalibrationResults.plot()`](https://lumicks-pylake.readthedocs.io/en/latest/_api/lumicks.pylake.force_calibration.power_spectrum_calibration.CalibrationResults.html#lumicks.pylake.force_calibration.power_spectrum_calibration.CalibrationResults.plot). |
There was a problem hiding this comment.
| * Allow showing the ranges that were excluded from a power spectrum by passing `show_excluded=True` to [`PowerSpectrum.plot()`](https://lumicks-pylake.readthedocs.io/en/latest/_api/lumicks.pylake.force_calibration.power_spectrum.PowerSpectrum.html#lumicks.pylake.force_calibration.power_spectrum.PowerSpectrum.plot). | |
| * Allow plotting the ranges that were excluded from fitting and the active calibration peak when plotting a calibration result using `show_excluded=True` and `show_active_peak=True` in [`CalibrationResults.plot()`](https://lumicks-pylake.readthedocs.io/en/latest/_api/lumicks.pylake.force_calibration.power_spectrum_calibration.CalibrationResults.html#lumicks.pylake.force_calibration.power_spectrum_calibration.CalibrationResults.plot). | |
| * Show the ranges that were excluded from a power spectrum or calibration fit by passing `show_excluded=True` to [`PowerSpectrum.plot()`](https://lumicks-pylake.readthedocs.io/en/latest/_api/lumicks.pylake.force_calibration.power_spectrum.PowerSpectrum.html#lumicks.pylake.force_calibration.power_spectrum.PowerSpectrum.plot) or [`CalibrationResults.plot()`](https://lumicks-pylake.readthedocs.io/en/latest/_api/lumicks.pylake.force_calibration.power_spectrum_calibration.CalibrationResults.html#lumicks.pylake.force_calibration.power_spectrum_calibration.CalibrationResults.plot). | |
| * Plot the active calibration peak for a calibration result using `show_active_peak=True` with [`CalibrationResults.plot()`](https://lumicks-pylake.readthedocs.io/en/latest/_api/lumicks.pylake.force_calibration.power_spectrum_calibration.CalibrationResults.html#lumicks.pylake.force_calibration.power_spectrum_calibration.CalibrationResults.plot). |
| import matplotlib.pyplot as plt | ||
|
|
||
| self.ps_data.plot(label="Data") | ||
| if show_active_peak and not hasattr(self.model, "output_power"): | ||
| raise ValueError("This is not an active calibration") |
There was a problem hiding this comment.
nit: I wonder if this should be an AttributeError since the root problem comes from not having the output_power attribute. Could also include in the Raises section of the docstring but we're not super consistent with this
There was a problem hiding this comment.
I was doubting about this too, but the error on the user side in this case is that they specified an incorrect value (they did not attempt to access this attribute themselves). That said, I will add a raises section, since it's generally a good idea to mention those.
| return ( | ||
| np.insert(frequency, nan_insertion_points, np.nan), | ||
| np.insert(power, nan_insertion_points, np.nan), | ||
| frequency[break_idx - 1], | ||
| power[break_idx - 1], | ||
| ) |
There was a problem hiding this comment.
it might be worthwhile commenting or adding to the docstring what exactly is returned from this function as it's not immediately clear from quickly inspecting the code
|
|
||
| self.total_samples_used = total_samples_used | ||
| self._raw_variance = variance | ||
| self._src_num_points_per_block = num_points_per_block |
There was a problem hiding this comment.
since this is private anyway, what's the src prefix for/mean?
There was a problem hiding this comment.
It stands for source and was a reminder that that refers to the num points per block of the source material (pre blocking). That said, it is probably superfluous, since you can so clearly see it in the property already.
| return self.sample_rate / self.total_sampled_used * self.num_points_per_block | ||
| return self.sample_rate / self.total_samples_used * self.num_points_per_block | ||
|
|
||
| def _apply_transforms(self, data, with_exclusions=True, downsample_data=True) -> np.ndarray: |
There was a problem hiding this comment.
I think a little docstring would be helpful here as it's not apparent to those of us that haven't delved deep into the intricacies of this math what transform needs to be applied or what it means
There was a problem hiding this comment.
Fair point! I'll add a docstring.
|
|
||
| def plot(self, **kwargs): | ||
| def plot(self, *, show_excluded=False, **kwargs): |
| plt.plot(self.frequency, self.power, **kwargs) | ||
| if show_excluded: | ||
| if self._downsampling_factor > 1: | ||
| plt.plot(self.unfiltered_frequency, self.unfiltered_power, label="_") |
There was a problem hiding this comment.
I had no idea that you could exclude labels from the legend like this 🤯
There was a problem hiding this comment.
Yeah. It's pretty nice. :)
JoepVanlier
force-pushed
the
lazy_power
branch
from
1c92eaf to
2c68786
Compare
Three reasons for doing this - A simpler constructor will allow easier construction of tests. - Keeping track of the high frequency data and performing the downsampling and exclusion/fit ranges lazily will allow us to do multiple analyses without the requirement to always recompute the expensive part (the FFT). - Keeping track of the uncut data will allow us to plot what we excluded.
allow plotting exclusion ranges and active calibration peak
JoepVanlier
force-pushed
the
lazy_power
branch
from
2c68786 to
345b72c
Compare
Why this PR?
PowerSpectrum. The benefits of this may not be evident in this PR, but a followup one will have pathological spectra constructed directly rather than ifft'ing the spectrum only to immediately fft it again.Tradeoffs
Increased size
Keeping the high frequency spectrum around does come at a cost, namely the increased memory footprint. For a typical calibration (100 kHz sample rate) this is about 4 MB worth of data per axis for passive, 8 for active.
Downsampling and frequency exclusion ranges
There is one annoying issue with showing the frequency exclusion ranges. Exclusion ranges are applied prior to downsampling, which means that downsampling can subsequently pull a point outside the exclusion ranges into it. This is not really a bug, as this allows us to make cuts smaller than the blocking size, but it can look weird.