Jupyter4Science / scipy2024-jupyter-widgets-tutorial

SciPy2024 Tutorial on "Building Complex Web Applications with Jupyter Notebooks"
Apache License 2.0
5 stars 6 forks source link

Updated Widgets notebooks #8

Closed JuanCab closed 3 months ago

JuanCab commented 3 months ago

This is a first attempt at merging the widgets notebooks from Matt and Nicole's original widgets walkthrough. I did restructure things a bit. Still a few things to do here, so this is a draft PR for now.

JuanCab commented 3 months ago

OK, I have reworked the widgets notebooks. Honestly, the widget content is very similar to before with a few nips and tucks. I did spend some time adding more explanation of what was going on and I did use ipydatagrid to display the pandas data frame, since it stays one size.

mwcraig commented 3 months ago

One quick question before I dive into a detailed review later today. Should the data be changing? I would think that we want the input temperature data to stay the same...

JuanCab commented 3 months ago

One quick question before I dive into a detailed review later today. Should the data be changing? I would think that we want the input temperature data to stay the same...

All I did was update the datafile to the most recent version of the NASA data, so it runs through 2023 instead of 2020. Other than updating the datafile, there is no 'change to the input data' in the sense of changing data mid-processing.

Please clarify if I am not understanding your concern.

JuanCab commented 3 months ago

OK, I pushed some updates to address all of Nicole's comments on the widgets notebook including:

mwcraig commented 3 months ago

All I did was update the datafile to the most recent version of the NASA data, so it runs through 2023 instead of 2020.

Ah, I see that now. Not surprised that some of the earlier data was updated too -- I was originally thrown off by changes in data as far back as 1880.

Definitely good to update with the latest data.

JuanCab commented 3 months ago

Ah, I see that now. Not surprised that some of the earlier data was updated too -- I was originally thrown off by changes in data as far back as 1880.

Yeah, I noticed that too but assumed some re-reduction of old data had tweaked the values.

JuanCab commented 3 months ago
  • In 02b_widgets should the minimum window size be 2 instead of 1? I guess this would apply earlier too.

I think the smoothing algorithm does fit with order 0, but it doesn't do anything. I'll see about making this change throughout.

JuanCab commented 3 months ago

Matt's issues that I have addressed (since the PR comments interface isn't letting me address these individually):

In 02a_nbdev a few suggestions/comments

  • I found I needed to export the imports at the top, the first time they are loaded, rather than the cell of imports that comes after the definition of the answer magic. If I didn't do that then the next notebook raises an error on importing dashboard because register_cell_magic is being used before it is imported.

Agreed, and fixed.

Maybe move the paragraph that starts "Isn't nbdev great!" to a later notebook, after we have finished building the dashboard? It seems premature at this stage. 02d_dashboard might be a good place to put it.

Done.

In the first place where you use the %answer magic point out that you need to execute that cell twice: once to load the solution and again to run the solution. One visual cue that it needs to be rerun is that the cell label turns yellow.

Done.

In 02b_widgets should the minimum window size be 2 instead of 1? I guess this would apply earlier too.

I changed both window_size and poly_order to reflect an updated idea that the polynomial order should be at least 1 which mandates a window size of 2 or above.

Does the %export magic get used anywhere? Could we leave it out?

It is not used anywhere I can see, it looks like it may have been meant by @nicole-brewer as a way of exporting notebooks in nbdev with solutions, but not sure...

Maybe add a brief exercise here asking them to pick a widget (any slider, dropdown or text entry box) and ask them to make and display it. Should just be a copy/paste from the linked documentation.

Done.

Do we use several algorithms? I'm happy to go there, especially as an exercise at the end, but I think right now there is only one smoothing algorithm.

I reworded this to state "smooth the data with an algorithm with different smoothing parameters"

How about adding a number 3 too:

  1. Only changes to the notebook that have been saved to disk will be written to the Python file..

Done.

Can we add some kind of indicator that this is an exercise? I've been having a little trouble following when I'm supposed to do something vs just run the code already there.

I have added an EXERCISE (all caps) to indicate exercises (I did this to all these notebooks). In the process I removed the answers that I forgotten to remove from 02c_layout.ipynb 🤦🏼

Maybe mention that the order matters: if you try to set the minimum first there will be an exception because the minimum you are trying to set is larger than the maximum. I think it wopuld also be fine to not mention it and just discuss it when it comes up in the room.

Let's wait and see if this happens to address it.

I don't think we need to point this out, but the Layout is itself a widget, so you can observe changes in its properties if you want.

I added a "PRO TIP" about this...

Why are window_size and poly_order displayed again here? Seems like making the HBox is the next step....

Removed them.

mwcraig commented 3 months ago

Does the %export magic get used anywhere? Could we leave it out?

It is not used anywhere I can see, it looks like it may have been meant by @nicole-brewer as a way of exporting notebooks in nbdev with solutions, but not sure...

There is a -e option to the answer magic that calls export so we should leave it in rather than trying to edit the code for the magics.

JuanCab commented 3 months ago

OK, I have addressed all of @mwcraig recent feedback EXCEPT where not marked resolved. It was a few small items.

JuanCab commented 3 months ago

There may still a couple of minor things here or there, but I think this is ready to merge. Any remaining polishing will be easier in a new pull request.

Thanks. Merging.