Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Conversation

@Matthew-Andrew
Copy link
Contributor

@Matthew-Andrew Matthew-Andrew commented Sep 29, 2020

This adds a toy example of using ipywidgets in the example sans reduction.

At ISIS most of the options for a reduction are defined in the userfile which is provided to users by instrument scientists. The only options which tend to change between every reduction are the sample, transmission and background run numbers. I've therefore taken the minimal approach of providing a simple interface to load and process sets of data and a further simple interface to plot the resultant data arrays. A final example which might be useful is an easy way to save data out at the end.

I'm slightly uncertain what the best way to go here is in terms of workflow. So comments and ideas most welcome.

To test run the python notebook (you may have to run make_config.py first to set up the file paths).

@Matthew-Andrew Matthew-Andrew added the question Further information is requested label Sep 29, 2020
Copy link
Member

@SimonHeybrock SimonHeybrock left a comment

Choose a reason for hiding this comment

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

Looks good. Some thoughts and comments:

  • Make sure to "restart and clear outputs" before committing the notebook.
  • If we want to show this to users, we should probably have all code in a .py, not in the notebook. As it is, it is probably too scary?
  • _repr_html_ of the scipp module shows something akin to Mantid's workspace list. Can this be included in a widget somehow? Or just directly as a cell, before the plotting cell, so users can see what they could plot?
  • Should we make a really simple example that we can share with people, that works without data files? For example, first a "load" widget, which just makes some simple data (np.linspace and some random noise for example), a processing widget that "processes" the data (just apply a square), and a third to plot make the plot?
    • We could then, as a second step, do this for the SANS reduction, but at that point it would probably make sense to cleanup what we have now and move the code to a module, so we do not need to duplicate the SANS code in the notebook?
  • It is nice that the plot widget can be used to plot different things (i.e., the notebook is not linear in that sense).

@SimonHeybrock
Copy link
Member

Maybe one more thing we could demonstrate is how we can have an equivalent to Mantid's algorithm dialogs? For example, we could have a simple widget for a unit conversion, with a drop-down list of potential target units?

@Matthew-Andrew
Copy link
Contributor Author

Matthew-Andrew commented Oct 1, 2020

I have added a new example which addresses these points and I think better suits the requirements of the exercise. It contains three widgets one of which creates some data, a second which allows conversions and a third which displays the currently available scipp objects and allows you to plot them.

@Matthew-Andrew
Copy link
Contributor Author

One issue which I have not yet solved is whether there is a nice way to update the list of scipp objects dynamically when they are added to the global namespace from a method which the plotting widget is not currently observing.

@Matthew-Andrew Matthew-Andrew force-pushed the add_graphical_example branch 3 times, most recently from ed2e216 to 0df6b79 Compare October 1, 2020 15:48
@Matthew-Andrew
Copy link
Contributor Author

Matthew-Andrew commented Oct 2, 2020

Note currently to get the plot widget to work you need to update the the _repr_html_ method in object_list.py to take an optional scope parameter. As shown below.

def _repr_html_(input_scope=None):
    import inspect
    # Is there a better way to get the scope? The `7` is hard-coded for the
    # current IPython stack when calling _repr_html_ so this is bound to break.
    scope = input_scope if input_scope else inspect.stack()[7][0].f_globals
    from IPython import get_ipython
    ipython = get_ipython()
    out = ''
    for category in ['Variable', 'DataArray', 'Dataset']:
        names = ipython.magic(f"who_ls {category}")
        out += f"<details open=\"open\"><summary>{category}s:"\
               f"({len(names)})</summary>"
        for name in names:
            html = make_html(eval(name, scope))
            out += f"<details style=\"padding-left:2em\"><summary>"\
                   f"{name}</summary>{html}</details>"
        out += "</details>"
    from IPython.core.display import display, HTML
    display(HTML(out))

@SimonHeybrock
Copy link
Member

Works, even with the hidden/unintentional(?) feature that you can put something like test['sample'] into the plot widget to only plot one dataset item.

Questions (not necessarily requests to do this here and now):

  • Is there a nice way to hide the inputs of the cells that display the widgets?
  • Should we change the "create" widget to be a fake "load" widgets, i.e., replace the "dims" and "size" inuts be "filename" (which would be ignored).


def _on_button_clicked(self, b):
self.output.clear_output()
with self.output:
Copy link
Member

Choose a reason for hiding this comment

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

All this catching of matplotlib output should become much easier once the new plotting has been merged, since the figure will now itself be a widget, that can be placed in any widget box.

"plot_widget = PlotWidget(globals())\n",
"data_creation.subscribe(plot_widget)\n",
"data_conversion.subscribe(plot_widget)\n",
"disp.display(plot_widget)"
Copy link
Member

Choose a reason for hiding this comment

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

Just tried the notebook but got this error
Screenshot at 2020-10-06 09-19-23

Copy link
Member

Choose a reason for hiding this comment

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

but maybe this is reated to Simon's other PR?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry just seen your comment above...

"outputs": [],
"source": [
"data_creation = DataCreationWidget(globals())\n",
"disp.display(data_creation)"
Copy link
Member

Choose a reason for hiding this comment

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

why do you need the disp.display() here?
why not simply

data_creation = DataCreationWidget(globals())
data_creation

?

@nvaytet
Copy link
Member

nvaytet commented Oct 6, 2020

Regarding the graphical_reduction:

I'm not really a fan of the

reduction_widget = ReductionWidget(globals())

syntax. As you say, there is some brittle mechanism requiring some variables to exist in the notebook.
They should instead be initialised to a default behind the widget interface.

Also, there is a big chunk of python code just before calling the graphical interface, which will look scary to non-programmers, and makes a big dent in what we are trying to show: that GUIs can be created in Jupyter. I guess there should be an input field for all these variables. They should then all be passed to the reduction backend.

@Matthew-Andrew
Copy link
Contributor Author

Matthew-Andrew commented Oct 6, 2020

@nvaytet I was attempting to minimize the number of fields in the widget and had a vague idea that scientists could set up the variables which were not going to change very much in a scripted fashion and then use the interface merely for what changes run to run but you are correct that this somewhat defeats the point of the example and if we are doing it we should pass the options explicitly to the widget. Whilst I don't think in the long run we want to add graphical options for all sans reduction options I think the minimal subset here should be fine.

Another option is we could institute some form of user file mechanism like the sans group do at ISIS. This is basically a file in what I believe is currently a toml format that contains all the standard settings for a reduction. These files are created for users by the instrument scientists and then a reduction is hopefully as simple as loading the user file containing all the settings, specifying run numbers and pressing go. Power users still like to modify things and play around with the nitty gritty of reduction parameters but these are probably the ones we should be encouraging to script. Do we want to bite the bullet and use a toml based user file? Or is that not a convention the ESS SANS scientists are likely to adopt.

@nvaytet
Copy link
Member

nvaytet commented Oct 6, 2020

Yes, sounds good with some sort of configuration files.
For this example, I think all the python code above should be hidden in the widget creation.
Users should only have to enter run numbers or file names, and assume that all paths are set (like they always do), and a default calibration file is being used.

@Matthew-Andrew
Copy link
Contributor Author

Ok I'll have a go updating to use configuration files. The other question I had for @nvaytet in particular was what extra fields you think might be useful to be able to specify from a plotting perspective.

The other thing I want to look into is providing better feedback during long operations to the user so they know what is going on. Currently you just have to rely on jupyter telling you whether the kernel is working or not which isn't that clear.

@nvaytet
Copy link
Member

nvaytet commented Oct 6, 2020

The other thing I want to look into is providing better feedback during long operations to the user so they know what is going on. Currently you just have to rely on jupyter telling you whether the kernel is working or not which isn't that clear.

I know ipywidgets as some progress bar widgets (IntProgress i think), but i don't know how easy it would be to hook that in.
We don't have any progress reporting in scipp. If you are running a Mantid alg, maybe there is a way to tap into the progress reporting?

Sounds quite complicated to me, and probably not wirth it at this stage. For this example, i would just use some data that is small and takes no time to reduce.

@SimonHeybrock
Copy link
Member

I think (hope) we are far from the point were we need to or want to make a single big reduction widget. My favored approach (medium-term) would be:

  • Provide a few key widgets, e.g., for loading, running a script, and plotting.
  • Make these widgets configurable, so they can be used as building blocks for a reduction notebook.
  • Make a nicely structured notebook, with a mix of markdown, widgets (with hidden input for creating the widgets), and code.

What do I mean be configurable widgets? For example, we want a generic "load" widget, but in practice users would like to simply enter their run number. The config of the instrument would be part of the notebook, or an instrument-specific helper lib. For example:

# create load widget with input box for run number
load = sc.widgets.load(path='/path/LARMOR/{}.nxs', inputs=['run-number'])

In a similar way, I would imagine a generic "reduction" widget:

# create reduction widget with two input boxes for names of loaded sample
# and background runs, and one for a reduction parameter (wavelength range)
from ess import loki
reduce = sc.widgets.reduce(run=loki.reduce, inputs=['sample', 'background', 'wavelength-range'])

@SimonHeybrock
Copy link
Member

Essentially, I think what I am saying is that the widgets should be quite simple and generic, and we'd use things such as https://docs.python.org/2/library/functools.html#functools.partial to set them up in a workflow/instrument-specific way, binding configurable input fields to function args.

@Matthew-Andrew
Copy link
Contributor Author

@SimonHeybrock I was thinking along similar lines to this. Is it worth demonstrating the ability to produce generic widgets here as I don't think it should take much extra work to make the current examples generic as you suggest.

@SimonHeybrock
Copy link
Member

@SimonHeybrock Is it worth demonstrating the ability to produce generic widgets here as I don't think it should take much extra work to make the current examples generic as you suggest.

I think so, it may be even less code, since it avoids a lot of the specific widget setup you did now?

@Matthew-Andrew
Copy link
Contributor Author

Matthew-Andrew commented Oct 6, 2020

I felt that sorting out how we want to handle simple widgets was important before we tried an example with a full reduction. I've therefore removed the SANS example for now and just left the updated toy example. This has left some unnecessary commits in the history but didn't want to force push too much whilst under review. Would suggest a rebase/squash before merge though.

This has been updated as Simon suggested to use more generic widgets. This leads to a slight loss in functionality in terms of input validation and auto-complete but these aren't drastic and there is no reason we couldn't pass in validators with our required inputs if we wished.

Taking transformation widget as an example the basic design idea is that you pass in the following:

  • The global scope, this is needed to retrieve and set items in the notebook. You could pass a getter and setter method instead but feels like if you are passing a dicts get_Item and set__item methods you may as well pass the whole thing.
  • A callable with a signature such as def convert(data, from, to)
  • The name you want to give to the operation
  • A dict of inputs in the form {key_word_arg: converter from string representation} . It's here we could pass i validators for each input if we wished. Was uncertain whether we wanted to complicate the api further or not.

@SimonHeybrock
Copy link
Member

Really nice! 👍

Is there any reason we even need distinct widgets for loading and processing? Apart from making a wrapping "load" function/lambda does the run-number conversion? Or maybe more generally, could have have all widgets accept a converter applied to their argument before calling the functions? Kind of like the validator you mention, except that it would not just validate but also transform? Basically, do what you did for LoadWidget, but replace filename_converter by list/dict of converters for each arg (and do nothing if there is no converter)?

@Matthew-Andrew
Copy link
Contributor Author

Matthew-Andrew commented Oct 7, 2020

Currently all the input widgets do have a converter applied to their arguments before applying to the function, so you are correct the conversion from run number to filepath is not a blocker to having the load and conversion widget being the same.

The reason I ended up splitting the load and conversion widgets was more to do with how the outputs are handled. For the converter you specify an output name whereas for the load widget you derive the output name from the filename. When I briefly tried to write this behavior in a generalized format specifying the outputs became quite complicated. It has just occurred to me though that if you have a function which returns more than one object that cannot be handled by the current setup so I'll have another look at the best way to specify how to handle the outputs of the function.

@SimonHeybrock
Copy link
Member

The reason I ended up splitting the load and conversion widgets was more to do with how the outputs are handled. For the converter you specify an output name whereas for the load widget you derive the output name from the filename. When I briefly tried to write this behavior in a generalized format specifying the outputs became quite complicated. It has just occurred to me though that if you have a function which returns more than one object that cannot be handled by the current setup so I'll have another look at the best way to specify how to handle the outputs of the function.

Maybe we should just always require specifying the output name? If the function return multiple objects this will simply reference a tuple.

@Matthew-Andrew
Copy link
Contributor Author

Good point on the tuple, this will just be handled by python. If we return a tuple of scipp objects and assign this into the global namespace this won't be picked up by the scipp module html representation. I don't know how common a case is where we return multiple scipp objects from a function is though?

I think it is a good idea to allow the users to specify the output name they want their loaded data to have. This would allow specific run numbers to be loaded and given names such as sample/vanadium. This may lead to a slight reduction in usability if someone is loading quite a lot of runs at once and auto-populating the output would probably be ideal. This case may be best solved by some sort of separate batch processing widget where you pass in a range of inputs and supply a function to generate the output name from input name.

In any case I'll add in some validation and harmonise the loading/processing.

@Matthew-Andrew
Copy link
Contributor Author

In fact the more I think about it a single process/batch process is a more sensible division that load/transform was.

@SimonHeybrock
Copy link
Member

Sounds sensible 👍 (but let us postpone the batch processing).

Matthew-Andrew and others added 10 commits October 7, 2020 12:15
This updates ProcessWidget so that the conversion functions
provided for input cells can optionally do a validation check
and throw a ValueError if this fails. This will then be handled
as a user input error by the widget. An alternative route to
pass in optional validators would be to have a seperate dict
sharing keys with the input dict which specifies options to use.
I went for this method over that because conversion and validation are often
closely linked.
Added ability to optionally specify autocomplete values for cells.
This is demonstrated for the data inputs and dimension values in the
conversion widgets. Note however that without linking up some observer
you have to re-run the cell to get the latest data to properly appear as
options. In the long term if may be worth having a notifier control object
which all widgets which create scipp objects register with and can then subscribe
to if they wish to know when scipp objects are created by have not done so currently.
Matthew-Andrew pushed a commit to scipp/scippwidgets that referenced this pull request Oct 30, 2020
Add initial widgets and converters, this has already been reviewed here scipp/ess-legacy#30 . So mergeing for now to create initial tag.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

question Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants