-
Notifications
You must be signed in to change notification settings - Fork 1
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Remove old mpl-specific figure code and saving calls; now only uses p…
…lotly
- Loading branch information
1 parent
a8ae719
commit 0d05160
Showing
1 changed file
with
4 additions
and
82 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0d05160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure i understand this commit-- it removes code to generate the figure with MPL, but it does not add code to make the figure with plotly-- or am i missing somethig?
0d05160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also-- does it mean that the current notebook (ie the original one) does not generate the plots anymore?
0d05160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, i think i understand now-- the figs are generated AT THE END of the notebook:
0d05160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, this is quite different from the 'regular' notebook (where people see the fig right after the processing)-- but i understand this was done to merge the neurolibre and colab notebooks into a single notebook-- weighting pros/cons, i guess the merge is probably a better idea
0d05160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one problem with that, though, is that it makes the notebook much more complicated to read for newbies, and one of the purpose of the notebook was 'reusability'-- eg: the native version of the notebook (ie: the 'easy to understand one') is currently being reused for another project: spinal-cord-7t/coil-qc-code#7 (comment). So i'm not sure i want the 'neurolibre-like' notebook to be used as a template for spin off projects-- tagging @nstikov so he is aware of this collateral issue
@mathieuboudreau i'm not sure what's the best approach at this point-- maybe keeping two notebooks out of sync was the better approach... i dunno. we should discuss. Also tagging @evaalonsoortiz and @Kyota-exe so they are aware
0d05160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcohenadad So, my old way, both were "generated" when the notebook is run, but only the plotly one is displayed in the HTML Jupyter Book. It is a bit more complicated as you noticed, as I took the data for the plot just before MPL plots it and saved it in the variables to be used with plotly later, so that if anything substantial (i.e. not cosmetic) changes in the data processing, both figures would get updated at tyhe same time.
That's a good point, I guess the issue originates with the fact that you already had your paper-quality plots ready; could have just been skipped if you wanted in this case (or as we almost did, convert completely from MPL to Plotly).
Yes I strongly agree with this point, in general with Plotly. Recall, the cNeuroMod book that Kiril had built also had issues with reusability / duplicate code, and it took me (which for the sake of neurolibre+plotly, we'll say I'm an expert) a considerable amount of time refactoring it into something that was fairly reusable/flexible (but still quite complicated code)
0d05160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was so that 1) the figures had the same "order" as for your figure, but also 2) because you were plotting the A and B columns in two completely different parts of the figure using MPL, and I felt you wanted one figure showing both next to each other., so this is the solution I decided on.