-
Notifications
You must be signed in to change notification settings - Fork 526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Paper: Supporting Greater Interactivity in the IPython Visualization Ecosystem #912
Conversation
Curvenote Preview
|
It is a pleasure to review your paper. I have thoroughly enjoyed reading your work and find it innovative. The paper follows a logical flow, explaining the importance of interactive visualization, the limitations of IPyWidgets, the benefits of IPyVuetify, and finally introducing IPyOverlay as a solution. It explains IPyWidgets, IPyVuetify, and how IPyOverlay addresses their limitations. The example use case effectively illustrates the library's practical application. However, I noticed that some sentences are a little complex and may cause confusion for some readers. Simplifying these sentences by breaking them down into smaller parts could enhance readability and ensure that your key points are communicated clearly to a wider audience. For instance, in the Introduction section, the sentence: Could be revised to: Similarly, in the IPyWidgets section: Could be revised to: |
The paper looks good to me. The writing is clear and concise. I can appreciate the utility of IPyOverlay. I am not an expert in visualizations. But should there be more details on the implementation of the library? There is a minor rendering issue in the caption of |
@ktzhao @ritwit If you would please, add line by line suggestions per the GitHub PR review process: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request |
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.
This paper was genuinely interesting to read. I had run into this issue of trying to have a matplotlib widget respond to show an inset of the region clicked, the authors went ahead and developed a solution!
I like the fact that this paper doesn't try to operate in a vacuum by presenting IPyOverlay alone as a completed project. Instead, it provides a sufficient background into iPyWidgets, the challenges of details-on-demand GUI design presented by iPyWidgets and the development of both iPyVuetify and finally iPyOverlay as solutions to this GUI design goal. This helps the reader really grasp not only what iPyOverlay is adding to the iPyWidgets ecosystem but also why those features are truly important for the development of better data exploration tools.
Prose issues addressed in individual comments
The only consistent flaw in the prose in this paper is that the draft presented has a tendency toward wordy and run sentences. In many cases, the run on sentence could easily have been broken up into distinct sentences, each presenting a distinct idea. I tried to note as many of those wordy and run on sentences in the comments of my review.
Prose issues not noted in individual comments
I did also run proselint on the existing draft and it caught 3 real issues, only one of which are likely worth addressing. The correction worth addressing:
- In line 148 of main.md: proselint suggests 'interact' instead of 'interact with each other' since the 'with each other' is implied and redundant.
The corrections that may not matter given the conversion of this document into HTML.
- proselint noted inconsistent spacing after period (1 vs. 2 spaces).
- proselint noted the use of straight quotes "" instead of quotes “”.
** Code review**
Code Block 1: No issues
Code Block 2: The use of ellipsis in the lines
parameter1 = ipw.FloatSlider(...)
parameter2 = ipw.IntText(...)
parameter3 = ipw.IntSlider(...)
and the mention of functions that were not actually in the provided in the event handler functions made the function untestable. I made changes to the code so that it can be run and would suggest something like
import ipywidgets as ipw
# initialize the widgets
graph1_out = ipw.Output()
graph2_out = ipw.Output()
parameter1 = ipw.FloatSlider(min=0, max=10, value=1)
parameter2 = ipw.IntText(min=0, max=10, value=1)
parameter3 = ipw.IntSlider(min=0, max=10, value=1)
# create the dashboard layout, combining all the widgets
dashboard = ipw.VBox([
ipw.HBox([graph1_out, graph2_out]),
ipw.HBox([parameter1, parameter2, parameter3]),
])
# event handler functions
def on_parameter1_change(change):
graph1_out.clear_output(wait=True)
with graph1_out:
print(f"parameter 1 is {change['new']} and parameter 2 is {parameter2.value}.")
def on_parameter2_change(change):
graph1_out.clear_output(wait=True)
graph2_out.clear_output(wait=True)
with graph1_out:
print(f"parameter 1 is {parameter1.value} and parameter 2 is {change['new']}.")
with graph2_out:
print(f"parameter 2 is {change['new']} and parameter 3 is {parameter3.value}.")
def on_parameter3_change(change):
graph2_out.clear_output(wait=True)
with graph2_out:
print(f"parameter 2 is {parameter2.value} and parameter 3 is {change['new']}.")
# attach event handlers
parameter1.observe(on_parameter1_change, names=["value"])
parameter2.observe(on_parameter2_change, names=["value"])
parameter3.observe(on_parameter3_change, names=["value"])
dashboard
Code Block 3: I would have included the code to display the button, which is shown in Figure 5 but not in the actual code. Again, I made the changes myself and would suggest something like:
import traitlets
import ipyvuetify as v
class ColoredToggleButton(v.VuetifyTemplate):
color1 = traitlets.Unicode("var(--md-light-blue-500)").tag(sync=True)
color2 = traitlets.Unicode("#AA6688").tag(sync=True)
state = traitlets.Bool(True).tag(sync=True)
@traitlets.default("template")
def _template(self):
return """
<template>
<v-btn
width="300px"
:color="state ? color1 : color2"
@click="state = !state"
>Press to change color!</v-btn>
</template>
"""
button = ColoredToggleButton()
# Display button
button
followed by a mention of that running
button.color2 = "var(--md-green-600)"
will change the second color programmatically.
Code Block 4: I was disappointed that we were not provided with example iPyOverlay code both to show an example implementation of the Python package (although we were shown the Vue template for an overlay container in Code 4, I wasn't hable to test it with the code provided)
@JuanCab thanks for mentioning proselint, I didn't know this existed but I'll be making frequent use of it moving forward! I updated programs 2 and 3 with your suggested changes, definitely agreed it's a lot nicer to be able to directly copy the snippet and have a functioning example in a notebook. It's also a good point that we don't ever include an example of using ipyoverlay itself! I had previously been worried about running into the word count limit, but I think I'm quite a bit under. I've been wanting to work up a good concise example notebook for pop-out subplots to include in the repository itself, once I do that I'll add a code snippet for it in the paper |
I apologize for the confusion, while I can review individual commits, apparently when I approve an individual commit, it just marks the entire PR as approved, not what I meant! |
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 have gone through all the commits through August 1, 2024 17:45 UTC and they all look good.
@@ -742,6 +742,67 @@ An activated context menu providing a selection of commands, which can be | |||
expanded well beyond the single possible action that can be taken from a left-click. | |||
::: | |||
|
|||
### Example Usage | |||
|
|||
A minimal code example using IPyOverlay's containers and connection lines is |
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.
Thank you for adding the code example!
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 like the code changes, they do make things a little clearer and easier to follow.
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 like the code changes, they do make things a little clearer and easier to follow.
If you are creating this PR in order to submit a draft of your paper, please name your PR with
Paper: <title>
. An editor will then add apaper
label and GitHub Actions will be run to check and build your paper.See the project readme for more information.
Editor: Charles Lindsey @cdlindsey
Reviewers: