Add simple example for scalar output. #15
Conversation
| @@ -0,0 +1,182 @@ | |||
| { | |||
There was a problem hiding this comment.
@jjaco2 You probably want to review the strings in this file. They may be over explanatory.
There was a problem hiding this comment.
See comment below on this -- I think this is an appropriate level of detail. We tend to under-explain on code examples, and I like opportunities to slowly change that.
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "output = int(numeric_param) + uniform(0,1)" |
There was a problem hiding this comment.
This cast may not be needed anymore.
| "source": [ | ||
| "# Simple Scalar Output Example\n", | ||
| "\n", | ||
| "In this example, we will define an integer parameter called `Integer input` and will output a floating point called `My scalar output` that can be bound to a numeric dashboard tile." |
There was a problem hiding this comment.
Maybe reword this slightly to be clearer about what this example helps the user do:
'This example shows how to create a scalar output [in Jupyter? not sure if that makes sense] that you can bind to a numeric dashboard tile in SystemLink.'
| "source": [ | ||
| "## Parameters\n", | ||
| "\n", | ||
| "The parameters block is where you define all of the inputs and outputs to the notebook. If you look at __Property Inspector >> Advanced Tools >> Cell Metadata__, while the `# Parameters` code cell block is selected, you will see the json that describes the inputs and outputs. Note that when you edit that json, you must click the small check mark under \"Cell Metadata\" to save the changes. In this example you can see we have configured a numeric input and a numeric output." |
There was a problem hiding this comment.
I actually don't think this is overexplaining at all, especially if this would be some people's first time doing this sort of thing, which I can see being true.
Only note here: json -> JSON file
| } | ||
| ], | ||
| "source": [ | ||
| "# Gluing the result list to the scrapbook is what actually allows you to bind the result to the numeric Dashboard tile.\n", |
There was a problem hiding this comment.
I am loving this scrapbooking/gluing language. I didn't realize this was a thing, and I? Am delighted.
| @@ -0,0 +1,182 @@ | |||
| { | |||
There was a problem hiding this comment.
See comment below on this -- I think this is an appropriate level of detail. We tend to under-explain on code examples, and I like opportunities to slowly change that.
alexweav
left a comment
alexweav
left a comment
There was a problem hiding this comment.
Looks really good, and this a much needed example. I had just a few style/nitpick comments.
| "source": [ | ||
| "# Gluing the result list to the scrapbook is what actually allows you to bind the result to the numeric Dashboard tile.\n", | ||
| "sb.glue('result', result)\n", | ||
| "result # this line is just here so you can see the result in Jupyter Hub and doesn't impact Dashboard." |
There was a problem hiding this comment.
| "result # this line is just here so you can see the result in Jupyter Hub and doesn't impact Dashboard." | |
| "display(result)" |
Jupyter injects the display() builtin which is automatically called on the last expression of every cell. If we want to be explicit about this, we could just directly call it ourselves rather than needing a comment to explain it.
| "source": [ | ||
| "# Simple Scalar Output Example\n", | ||
| "\n", | ||
| "In this example, we will define an integer parameter called `Integer input` and will output a floating point called `My scalar output` that can be bound to a numeric dashboard tile." |
There was a problem hiding this comment.
Should we standardize on example variable naming here? E.g. using "My integer input" or "Scalar output" instead?
| "outputs": [], | ||
| "source": [ | ||
| "import scrapbook as sb \n", | ||
| "from random import *" |
There was a problem hiding this comment.
import * is often discouraged in favor of either importing the module directly, or importing what you need from the module, like import random followed by random.uniform() or from random import uniform followed by uniform(). https://ni.github.io/python-styleguide/#O-1-6
There was a problem hiding this comment.
I'd also recommend moving the random import above the scrapbook import and adding a blank line between the two, since random is a standard library import which is typically lifted above 3rd party imports. https://ni.github.io/python-styleguide/#O-1-3
| ] | ||
| }, | ||
| { | ||
| "cell_type": "code", |
There was a problem hiding this comment.
It looks weird to have two code cells in a row without any description. Should we add markdown above this describing what this cell is doing? It would let the reader differentiate what's a parameter vs. what's implementation of their custom logic, in case they're using this as a template.
| "# Parameters\n", | ||
| "numeric_param = 1 # default value used when input is not provided by Dashboard\n", | ||
| "\n", | ||
| "result = [] # container for all of the notebooks results" |
There was a problem hiding this comment.
Papermill works by injecting a new cell containing parameter value overrides directly below the cell marked parameters. Therefore, it's a good practice to keep any code that's not defining a default value for a parameter outside of the parameters cell. You're technically safe in this particular instance, but if this line depended on the parameter above, you can end up with some really weird (and hard to debug) behavior since that code would be referencing the default value, not the passed-in value.
There was a problem hiding this comment.
Also this line feels like it belongs in the "Results" cell a few cells down.
| } | ||
| ], | ||
| "source": [ | ||
| "# Gluing the result list to the scrapbook is what actually allows you to bind the result to the numeric Dashboard tile.\n", |
There was a problem hiding this comment.
Note, you don't really have to do anything for this: Technically the styleguide recommends using 100 character lines, but that rule is particularly suited towards Python source files and makes less sense in notebooks. I don't think the conventions were really ever fully designed for notebooks because other rules don't apply whatsoever.
https://ni.github.io/python-styleguide/#F-1-2
Line length is fragmented in python and others agree on 120 (and this line is 119), so it might be fine to deviate here and keep it, since there's no reason to have such a short line length limit in a notebook outside of a .py file. Just something to be aware of.
| "# Scalar result needs to be added to the result list and glued to the scrapbook\n", | ||
| "# The json descriptor should match the descriptor in the outputs section in the parameters cell metadata.\n", | ||
| "result.append({\n", | ||
| " 'type': 'scalar',\n", |
There was a problem hiding this comment.
Via a really roundabout way, the styleguide also recommends double-quotes for Python strings. Technically it just recommends any style rule that black also specifies: https://ni.github.io/python-styleguide/#F-1-1 and black specifies double quotes. We should probably start using double quotes for new examples - I'm aware that some older product code does not yet follow the newer conventions.
This is another example of the styleguide not perfectly applying to notebooks, as black doesn't work on notebooks natively. However, there are ways of getting this to be a thing, ping me if you're particularly curious about python linting.
| " 'config': {\n", | ||
| " 'title': 'Scalar Output Title'\n", | ||
| " },\n", | ||
| " 'value': output\n", |
There was a problem hiding this comment.
| " 'value': output\n", | |
| " 'value': output,\n", |
black also recommends trailing commas in multi-line lists and dictionaries. There are a few other places where this applies as well.
3 participants
What does this Pull Request accomplish?
This adds an example for creating a scalar output that can be bound to a numeric dashboard tile.
Why should this Pull Request be merged?
This example helped a customer accomplish this task.
What testing has been done?
Used in product.