Firefly-1483: improve support for triview #58
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
robyww
force-pushed
the
FIREFLY-1483-triview
branch
from
b3c755e to
bf9764b
Compare
There was a problem hiding this comment.
This looks great! Triview mode works really well and it's good that it allows user to not concern with layout (like in slate). It's also good that it combines the results from two different notebooks to one client's triview now.
The naming of notebooks is confusing (it was already not meaningful with names like 'slate-demo', 'slate-explicit') but now they were harder for me to follow. Because after these changes, they're no longer "slate" but triview (default). Naming shouldn't really have mode in it, it should be just client_demo or api_demo, etc.
Also we have duplicate notebooks with lab_client instead of simple client - we should come up with a standardised naming scheme in general, for example notebooks. [Thinking more about it] we earlier had: make_client or make_lab_client, slate, and multi modes, but now we have: make_client or make_lab_client, triview or slate, single or multi client-view modes (so more permutations).
Rest all looks great except a few minor comments.
| @@ -0,0 +1,500 @@ | |||
| { | |||
There was a problem hiding this comment.
This file seems a copy of slate-demo-explicit, except with make_lab_client instead of make_client You might wanna rename the filename's last part from -slate-steps to -demo-explicit like the case with other file (it took me a while to follow)
Reply via ReviewNB
| @@ -17,31 +17,31 @@ | |||
| "## Setup" | |||
There was a problem hiding this comment.
| @@ -17,57 +17,31 @@ | |||
| "## Setup" | |||
There was a problem hiding this comment.
| @@ -0,0 +1,303 @@ | |||
| { | |||
There was a problem hiding this comment.
- firefly_client knows what sort of view it is using triview/slate/other - added change_triview_layout using new firefly dispatch - warnings and noop when wrong function is used - massive cleanup of notebooks - inclues reponse to feedback
robyww
force-pushed
the
FIREFLY-1483-triview
branch
from
abd0929 to
0b9ec49
Compare
Firefly-1483: change firefly_client to better support triview and plan to deprecate slate
see firefly PR: Caltech-IPAC/firefly#1557
see jupyter_firefly_extension PR: Caltech-IPAC/jupyter_firefly_extensions#29
Testing
FireflyClient.make_client()andFireflyClient.make_lab_client()slate.htmlwill put it is slate mode otherwise it will be in triview modejl-lab-extension-demo-explicit2.ipynbjl-lab-extension-slate-steps.ipynbbasic_3color.ipynbbasic-demo.ipynbslate-demo-explicit.ipynb