Skip to content
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

Add tutorial on constructing graph with nodes and interactive widgets #45

Merged
merged 18 commits into from
Oct 25, 2022

Conversation

nvaytet
Copy link
Member

@nvaytet nvaytet commented Oct 20, 2022

No description provided.

@nvaytet nvaytet marked this pull request as ready for review October 21, 2022 12:23
@nvaytet nvaytet requested a review from jl-wynen October 21, 2022 12:23
Copy link
Member

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to these changes: I get

WARNING: Invalid configuration value found: 'language = None'. Update your configuration to a valid language code. Falling back to 'en' (English).

This is about a setting in conf.py.

docs/tutorials/nodes-tutorial.ipynb Outdated Show resolved Hide resolved
"At the most basic level, a graph will contain a node (white rectangle) that provides the input data,\n",
"and a view (grey ellipse) which will be figure to display the data visually.\n",
"\n",
"![graph](_static/node_graph.png)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node_graph.png does not show up in the docs. I get a warning from Sphinx:

/home/jl/Work/plopp/docs/tutorials/nodes-tutorial.ipynb:: WARNING: image file not readable: tutorials/_static/node_graph.png

The path here needs to be relative to the notebook.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-organised the notebook so that I don't need the png anymore

"metadata": {},
"outputs": [],
"source": [
"pp.show_graph(a)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the graphs, it would be nice if users could name nodes (e.g. by overriding the id). As it stands, it is pretty difficult to understand what a graph does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See update. For now, I just went with setting the .name on the node after it is created.

"metadata": {},
"outputs": [],
"source": [
"fig"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The figures look a bit blurry. Is it possible / feasible to use PDFs instead of PNGs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not :-(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to fiddle with the dpi

"source": [
"When something changes in one of the nodes, all the nodes below it in the graph are notified about the change (the children nodes receive a notification, and they, in turn, notify their own children).\n",
"It is then up to each view to decide whether they are interested in the notification or not (usually, most views are interested in all notifications from parents).\n",
"If they are, they request data from their parent nodes, which in turn request data from their parents, and so on, until the request has reached the top of the graph.\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was explained earlier. Do we need it twice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the first explanation shorter so that it feels like the details aren't given twice.

"It is then up to each view to decide whether they are interested in the notification or not (usually, most views are interested in all notifications from parents).\n",
"If they are, they request data from their parent nodes, which in turn request data from their parents, and so on, until the request has reached the top of the graph.\n",
"\n",
"As a result, when the slider is dragged, the smoothing node `c` gets notified and tells the figure that a change has occured.\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a note that it doesn't work in the rendered docs and that this is not a bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"metadata": {},
"outputs": [],
"source": [
"a = pp.input_node(da)\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too happy with using single letter names here because it promotes bad practice. It is easy to follow the logic here because the examples are small and self-contained. But I think we should not show anti-patterns in tutorials.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good suggestion. Done

"fig2d = pp.figure2d(c)\n",
"\n",
"# Sum the raw data along the vertical dimension\n",
"d = pp.node(sc.sum, dim='yy')(a)\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did the earlier example use partial to assign sigma but here, it uses kwargs of node? I'd stick to the latter in all cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Dropping the need for partial further up, as node does this for you.

"id": "4b595fcb-02b8-4fa3-be16-3512e65b2c6a",
"metadata": {},
"source": [
"Because the slider in only affecting the left part of the graph,\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Because the slider in only affecting the left part of the graph,\n",
"Because the slider only affects the left part of the graph,\n",

"metadata": {},
"source": [
"Because the slider in only affecting the left part of the graph,\n",
"only the orange markers will update when we drag the slider."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit risky to talk about 'the left part of the graph'. The layout might change if the algorithm or image size is tweaked.

@nvaytet nvaytet changed the title Add tutorial on constructing graph with nodes with interactive widgets Add tutorial on constructing graph with nodes and interactive widgets Oct 25, 2022
"metadata": {},
"outputs": [],
"source": [
"ipw.VBox([slider, fig2d, fig1d])"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to start the slider at a different value than 1? As it stands, the lines look almost identical

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to be at 10 when created.

"id": "69510b9d-8bbb-4e7d-8c39-dde70d88b3db",
"metadata": {},
"source": [
"We make a node from the checkboxes widge using `widget_node` once again.\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"We make a node from the checkboxes widge using `widget_node` once again.\n",
"We make a node from the checkboxes widget using `widget_node` once again.\n",

"Because the slider only affects the smoothing part of the graph,\n",
"only the orange markers will update when we drag the slider.\n",
"\n",
"## Using `node` as a function decorator\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the section because it adds a little bit of complexity. But the decorator is only a small part of it. Maybe change it to something like 'Multiple view and multiple controls'.

@nvaytet nvaytet merged commit 8092dd5 into main Oct 25, 2022
@nvaytet nvaytet deleted the nodes_tutorial branch October 25, 2022 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants