Fixes to dynamical power spectrum + Dynamical Cross spectrum

[matteobachetti]

• Improves performance and consistency when using events (uses the internals already tested in AveragePowerspectrum to create light curves dynamically for each segment, rather than allocating the full light curve on the spot)
• Resolves a bug when rebinning in time (it relied on dt, which was calculated in a weird manner)
• Makes tests a little more understandable, I hope

BONUS: added Dynamical Cross spectrum, because why not

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (68453c6) 96.17% compared to head (36c5c0c) 96.18%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #779      +/-   ##
==========================================
+ Coverage   96.17%   96.18%   +0.01%     
==========================================
  Files          43       43              
  Lines        7999     8020      +21     
==========================================
+ Hits         7693     7714      +21     
  Misses        306      306              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mgullik]

Hi @matteobachetti,

it's great that you updated this tool for the spectral-timing analysis.

I was wondering if it's worth adding dt as a parameter in the docstring of DynamicalCrossspectrum, specifying that dt needs to be an input in the case data1 and data2 are two EventList objects

Before calling _make_mastrix should init call the cross_gtis function? Or are we assuming that the gtis of data1 and data2 are the same?
Maybe AveragedCrossspectrum calls cross_gtis already, right?

The rebin_time function is a bit confusing.
If I had to think of rebinning in time the dynamical cross-spectrum I would think of changing the segment_size and not the dt, but maybe it's just me. However, it needs to be clearer in the docstring that the rebin_time function changes the Nyquist frequency and not the time axis of the dynamical cross-spectrum.
Actually, looking at the code, I'm not sure the rebin_data does the correct job, right?

The tests look good, but I'll check them more carefully next round.

[matteobachetti]

@mgullik yes, there is a confusion between two different meanings of dt (+segment_size), but rebin_time does what you say, not a rebinning of the light curve. I should have addressed your comments now, including renaming the input dt -> sample_time, which clears up at least part of the confusion.

[mgullik]

Hi @matteobachetti,

the review is completed, I suggested one last change to the docstring of the rebin_time. Sorry, probably too picky there

One thing, since you were so thorough in implementing the DynamicalCrossspectrum I'm wondering why don't you implement an option to output a complex matrix with real and imaginary parts instead of the normalized squared absolute values of Fourier amplitudes? In this way, it's even possible to create a dynamic lag frequency spectrum (boom!) However, not something to hold this PR longer.

Another very small thing about the tests, I noticed is that you always run the tests on DynamicalCrossspectrum with the same lightcurves for data1 and data2. Does it make sense to test it with two different light curves?

[matteobachetti]

As far as I can see, I'm not using the squared amplitudes anywhere, and indeed, the Dynamical Cross spectrum has complex values when you feed it with different light curves in the two channels. I added a test to show explicitly that