lkorczowski / Tinnitus-n-Sleep

Detecting events in sleeping tinnitus patients
MIT License
1 stars 0 forks source link

having a clean and functional notebook #19

Closed lkorczowski closed 4 years ago

lkorczowski commented 4 years ago

Functionalities:

review-notebook-app[bot] commented 4 years ago

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

codecov[bot] commented 4 years ago

Codecov Report

Merging #19 into master will not change coverage by %. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #19   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          372       372           
=========================================
  Hits           372       372           

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 128c7ea...128c7ea. Read the comment docs.

lkorczowski commented 4 years ago

I'm soon done, I'm just looking how to properly annotate the figures but the final results will be great, e.g. https://matplotlib.org/3.1.1/gallery/subplots_axes_and_figures/axes_zoom_effect.html#sphx-glr-gallery-subplots-axes-and-figures-axes-zoom-effect-py

lkorczowski commented 4 years ago

@RobinGuillard I think I did enough in this PR, please review all but the NB.

For the NB, we will integrate your automatic report.

lkorczowski commented 4 years ago

Very interesting, but yet important changes need to be made: It would be nice to have the line of the signal more fine. To distinguish better

as NOT mentioned in the docstring (I forgot), additional arguments (**kwargs) are passed to plot. So color, linewidth, etc. are all valid parameters

In addition, as you can see, on the bottom plot, burst are not easily seeable: image I would be better if they could be in first plan.

Problem here is that the burst are very small in comparison of the whole recording. Also, double check that you used pltAnnotations on both subplots (in my examples I didn't because it was heavy)

Moreover, I haven't found for now a way to scroll "rapidely" on the zoomed version of the data. It is currently not envisageable to explore all the data at this speed.

As mentioned in the notebook, for now we use plt.set_xlim. I'll build a NB widget with a scollbar to do the same thing smoothly but it is not urgent.

Moreover, it seems we currently have only one color for all annotations, it would be more visual if we had a color per annotation type (perhaps an addition for dict_annotations?)

yep, for now, we need to call each plotAnnotation independently. The color is param fc

Your code definitely reveals a high level of mastery, and I am outpassed to understand most of what's happening in visualization.py and the corresponding outpass my level and my PR capability is limited.

anyway, reviewing code is hardcore. Thus the importance to focus on high quality pytest and thrust those.