Correlogram, Periodogram: Reimplementation in pyqtgraph

[janezd]

Issue

Widget uses highcharts, which we don't like.

Description of changes

Now it uses pyqtgraph, which we don't like less.

Includes

• [X] Code changes
• [X] Tests

[codecov-commenter]

Codecov Report

Merging #216 (6f50254) into master (026771c) will increase coverage by 1.34%. The diff coverage is 89.85%.

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
+ Coverage   70.21%   71.55%   +1.34%     
==========================================
  Files          28       29       +1     
  Lines        3566     3597      +31     
  Branches      613      620       +7     
==========================================
+ Hits         2504     2574      +70     
+ Misses        937      893      -44     
- Partials      125      130       +5     

Impacted Files	Coverage Δ
orangecontrib/timeseries/widgets/owperiodogram.py	68.42% <65.62%> (+68.42%)	:arrow_up:
orangecontrib/timeseries/widgets/owcorrelogram.py	93.87% <95.00%> (-1.65%)	:arrow_down:
orangecontrib/timeseries/widgets/owperiodbase.py	98.48% <98.48%> (ø)
orangecontrib/timeseries/timeseries.py	89.32% <0.00%> (-1.69%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 026771c...6f50254. Read the comment docs.

[ajdapretnar]

Periodogram issues:

• when using As Timeseries and changing the time attribute to any numeric attribute (say Low from YF), the widget crashes with:

  
  ----------------------------- TypeError Exception -----------------------------
  Traceback (most recent call last):
  File "/Users/ajda/orange/orange3-timeseries/orangecontrib/timeseries/widgets/owperiodbase.py", line 125, in _selection_changed
  self.replot()
  File "/Users/ajda/orange/orange3-timeseries/orangecontrib/timeseries/widgets/owperiodogram.py", line 47, in replot
  periods, grams = self.periodogram(attr)
  File "/Users/ajda/orange/orange3-timeseries/orangecontrib/timeseries/widgets/owperiodogram.py", line 36, in periodogram
  self._cached[attr] = periodogram_nonequispaced(times, x)
  File "/Users/ajda/orange/orange3-timeseries/orangecontrib/timeseries/functions.py", line 167, in periodogram_nonequispaced
  pgram = lombscargle(times, x, freqs)
  File "/Users/ajda/.pyenv-x86/versions/3.9.10/envs/py3.9/lib/python3.9/site-packages/scipy/signal/_spectral_py.py", line 148, in lombscargle
  pgram = _lombscargle(x, y, freqs)
  TypeError: Invalid call to pythranized function `_lombscargle(float64[:] (is a view), float64[:], float64[:])'
  Candidates are:
  
  - _lombscargle(float64[:], float64[:], float64[:])

---

Same if one uses instance order.
- Crashes on iris with:

-------------------------- AttributeError Exception --------------------------- Traceback (most recent call last): File "/Users/ajda/orange/orange3-timeseries/orangecontrib/timeseries/widgets/owperiodbase.py", line 125, in _selection_changed self.replot() File "/Users/ajda/orange/orange3-timeseries/orangecontrib/timeseries/widgets/owperiodogram.py", line 47, in replot periods, grams = self.periodogram(attr) File "/Users/ajda/orange/orange3-timeseries/orangecontrib/timeseries/widgets/owperiodogram.py", line 22, in periodogram if self.data.time_delta.is_equispaced: AttributeError: 'NoneType' object has no attribute 'is_equispaced'

- I liked the name of the x axis in the old widget. And thicker lines. I also think the grid in the background helped better determine the height of the bar. I can see lines from multiple attributes overlapped also in the old widget. Hover is probably difficult to implement, so I won't bother you with it. But it was nice to have it in HC and will be useful (if not inevitable) in the future (🤞) Line Chart implementation.

[ajdapretnar]

For Correlogram, I'd say similarly to above, I'd vote for thicker lines, grid and x axis name. The widget doesn't crash on the same data as Periodogram, so 👍 .

[janezd]

I think I've done everything except for x-axis title. It is always "Period" - and we know it is period, or else we don't know what the widget is doing. Is it really useful?

I've added horizontal grid. Do we also need the vertical?

Tooltips: yes, implementing them would require mapping coordinates in pyqtgraph, which is black magic. I'll wait for Vesna to finish line chart and then ask for her newly regained expertise. :)

[ajdapretnar]

The x axis name would be nice only when exporting the image. I agree it is not strictly necessary in the visualization, because indeed it is always period, but would this be just as evident in a report or a paper?

Horizontal grid suffices. I like it.

Ok, tooltips at a later date. :)

Just one more thing. It would be nice to unify the widget with Line Plot. Specifically in terms of zoom/pan/reset options. This is quite standard in most other Orange widgets.

[janezd]

For publishing in a paper, x-axis might have a different name, so let us not add it.

For toolbar, let us this be another issue. I am not very capable at scaling and moving pyqtgraph views. I think @VesnaT is. :) Also: the first button is inapplicable, and zooming would only work horizontally.

[ajdapretnar]

You are right, one button is redundant and the other only partially useful. I am just not sure the users will find the small A sign in the lower left corner. 😞 It would be nice to provide a clear way to reset the view. But I agree, having only the reset button would look strange.