JNB: Remove holoviews #228
Conversation
Codecov Report
@@ Coverage Diff @@
## master #228 +/- ##
==========================================
+ Coverage 92.45% 94.14% +1.69%
==========================================
Files 8 8
Lines 941 1350 +409
Branches 197 339 +142
==========================================
+ Hits 870 1271 +401
- Misses 34 40 +6
- Partials 37 39 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
scottclowe
force-pushed
the
jnb_no-holoviews
branch
from
da9258b
to
c911477
Compare
scottclowe
force-pushed
the
jnb_no-holoviews
branch
from
c911477
to
6fe91cb
Compare
There was a problem hiding this comment.
Overall happy with this, except for:
- There are several inconsistencies with how the plots are done across notebooks.
- Things can look a bit nicer with some minor formatting
- It would be nice to show how you can compare the central traces to the surrounding neuropil region traces
I am in progress of doing all of the above and will push them later this afternoon.
Okay, sounds good. |
| @@ -142,29 +144,40 @@ | |||
| "\n", | |||
| "trial = 1\n", | |||
| "\n", | |||
| "plt.figure(figsize=(12, 8))\n", | |||
| "plt.figure(figsize=(12, 4))\n", | |||
There was a problem hiding this comment.
Not sure why you want the figure to be so thin.
There was a problem hiding this comment.
Main reason: I just think it looks nicer this way. I prefer the time axis to be longer than the magnitude of the signals. This also makes the curve-sizes more consistent with the other plots.
| "# Fetch the colormap object for Cynthia Brewer's Paired color scheme\n", | ||
| "cmap = plt.get_cmap(\"Paired\")\n", |
There was a problem hiding this comment.
I was intentionally not using this colormap for this notebook because there is only the result line and no raw line to show. I did not want to needlessly add to the complexity of what is going on when it doesn't make the plots any clearer.
This change would make sense in #237, but not here.
There was a problem hiding this comment.
I think it looks much better this way, especially since we used the color code in for different ROIs before. Also, if you want this to be all blue than they should also be all blue in the other note-books, just have different shades for raw or separated. The colors don't really help with that. For me the colors are nice as it's immediately clear that different traces come from different cells.
There was a problem hiding this comment.
But I'm happy with both. I just figured you had not gone back to the functional notebook to update it with the nice plotting code from the other notebooks =).
There was a problem hiding this comment.
In any case with #237 this will all be moot.
| "The other signals are the (isolated) contaminants." | ||
| "The other signals are the (isolated) contaminants.\n", | ||
| "\n", | ||
| "We now first plot all the ROI signals for a given trial in a single plot:" |
There was a problem hiding this comment.
I don't like that the sentence starts with "now first", which are two (redundant? conflicting? unsure) ways of describing the current time. Pick one!
Also, I don't like that the sentence ends in a colon.
examples/Basic usage.ipynb
Outdated
| "# Plot the mean image for one of the trials\n", | ||
| "plt.figure(figsize=(176.0 / 25, 156.0 / 25))\n", |
There was a problem hiding this comment.
This is really overly complex. I guess you are trying to make the figure bigger without changing the aspect ratio? imshow will always have the right aspect ratio, so it is safe to just make a big square figure and then imshow will put the axes within that as best it can.
What you have now produces the same output as this:
plt.figure(figsize=(6.24, 6.24))
There was a problem hiding this comment.
I changed it to figsize=(7, 7)
There was a problem hiding this comment.
Yes I wanted to keep the aspect ratio. I did not know that this was automatically done!
| " # Labels and boiler plate\n", | ||
| " plt.ylim([-0.05 * roi_max[i_roi], roi_max[i_roi] * 1.05])\n", | ||
| " if i_roi == 0:\n", | ||
| " plt.ylabel(\"Signal intensity\\n(candela per unit area)\")\n", | ||
| " plt.ylabel(\"Trial {}\\n\\nSignal intensity\\n(candela per unit area)\".format(i_trial))\n", |
| "experiment = fissa.Experiment(images, [rois[:ncells]], output_folder)\n", | ||
| "experiment.separate()" |
There was a problem hiding this comment.
Sorry, I still don't have an environment that runs suite2p, so I rewrote these plots by running them in another notebook and copying the code over to this one.
Instead of a mix of (12, 4) and (12, 8).
scottclowe
force-pushed
the
jnb_no-holoviews
branch
from
e58bb14
to
8c21129
Compare
All holoviews plots replaced with matplotlib. Holoviews removed from requirements-plots.txt.
This change was made because:
+and*operators, and not relying on%magic, makes matplotlib more comprehensible for new users.It is unfortunate that we lose the cool sliders and dynamism of the holoviews outputs. I would be happy to include holoviews plots for this purpose in the "advanced" section of the object-oriented notebook, as there are advantages to the holoviews output particularly when exploring the data. To be clear, if this were in the object-oriented notebook already, I would have left it in. But we weren't making use of any sliders in that notebook, only in the SIMA, CNMF, and Suite2p notebooks. I would rather not introduce extra holoviews functionality in these notebooks, so now everything is just matplotlib.
I also changed some other things, like the alpha used in the second example for object-oriented notebook was decreased from the default so now there is a visible difference when the analysis is re-run with the different parameters.
@swkeemink Could you please pull this branch, run the notebooks locally, and see what you think of them? If you see some ways to make them more user-friendly, you can commit the additions and push to this branch.