scipp / essreduce

Common functionality for ESS data reduction
https://scipp.github.io/essreduce/
BSD 3-Clause "New" or "Revised" License
1 stars 1 forks source link

Improve Switchable Widget and handle intermediate parameters #74

Open YooSunYoung opened 1 month ago

YooSunYoung commented 1 month ago

Current Switchable widget is a check box that shows the underlying widget when it's selected.

@jokasimr suggested different way of handling parameter switch.

Instead of setting switchable to all the parameters, we can treat all parameters switchable, and have a menu of parameter from the workflow, just like we select output.

And if the parameter is selected, it should show the widget on the right side and the workflow-running widget should set the parameters.

Here is the conceptual drawing how it can look like:

image

image

jokasimr commented 1 month ago

I was thinking the "select parameters" section could just be one big multiple-select containing both parameters and intermediate parameters all together (all domain types in the workflow).

I don't see the reason for keeping the distinction between intermediary and non-intermediary parameters.

SimonHeybrock commented 1 month ago

One problem with this solution is that it is totally non obvious to the user which parameters can be set. Parts of defining "THE" parameter of the workflow is to document which inputs are required or options. Saying "any of the intermediate results of the workflow is an optional input" does not serve this important purpose.

jokasimr commented 1 month ago

Is your concern is that the user wont know what parameters to select in the big multiple-select list of parameters? That will be done automatically for them unless they manually change the settings that were made when they selected what outputs to target.

SimonHeybrock commented 1 month ago

Then I do not understand the suggestion. For example, above it says

Instead of setting switchable to all the parameters, we can treat all parameters switchable, and have a menu of parameter from the workflow, just like we select output.

Can you give an example of how this would work for, say, https://github.com/scipp/esssans/blob/1c407162a50f4e68b843899f55384f5c025e19e7/src/ess/sans/parameters.py#L99-L104?

jokasimr commented 1 month ago

It's really not different from how it works currently. The only change is that we add a multiple-select that determines what parameters are displayed in the ParameterBox widget (the parameter form).

Here's the UX I'm imagining:

  1. User selects workflow -> This updates the list of outputs they can select. (Just like today)
  2. User selects outputs to target -> This triggers two changes:
    • list of parameters in the parameters multiple-select is updated to contain all parameters upstream from the selected outputs in the task graph
    • the root nodes of the task graph are selected in the parameters multiple-select
  3. The user presses "Generate" button, the parameter form is populated with the parameters selected in the parameters multiple-select. (From this point everything is exactly like today)

If the user wants to set an intermediate value, they open the parameters-multiple select and selects that value, then they re-generate the parameter form.

Example - SANS direct beam

  1. User selects sans workflow and IofQ output.
  2. User opens parameters multiple-select.
  3. User selects "DirectBeam"
  4. User presses "Generate parameters" -> Parameter form is generated, the parameter form contains the direct beam widget.
YooSunYoung commented 1 month ago
SimonHeybrock commented 1 month ago

Ok, so we keep the Parameter.switchable field 👍

jokasimr commented 1 month ago

Ok, so we keep the Parameter.switchable field 👍

I think @YooSunYoung point was that removing switchable widget would make it easier to style the parameter form.

SimonHeybrock commented 1 month ago

I was refering to class Parameter, not the widget itself. Are you suggesting that there should be a different mechanism outside Parameter that defines which parameters of the workflow are switchable by default?

jokasimr commented 1 month ago

I was refering to class Parameter, not the widget itself. Are you suggesting that there should be a different mechanism outside Parameter that defines which parameters of the workflow are switchable by default?

No I don't have an opinion on that

jokasimr commented 1 month ago

For clarity here are a couple of screenshots illustrating the idea:

Screenshot from 2024-08-13 16-08-50 Screenshot from 2024-08-13 16-09-09

Ignoring the collapsed "Manually select parameters" accordion makes the interface the same as what we have today. If you open the "Manually select parameters" menu you can select intermediate values to be added to the parameters form.

SimonHeybrock commented 1 month ago

Sure, but to reiterate my question from above: How do you populate the "manually select parameters" list? If we use Parameter.switchable that is probably fine. If not, what is your plan?

We can chat in person today!

jokasimr commented 1 month ago

The plan was to populate the manually selected parameters list by just grabbing all domain types in the task graph (upstream from the target outputs).

But yeah using Parameter.switchable is of course another option.

Unfortunately I'm WFH today, but we can huddle.

SimonHeybrock commented 1 month ago

The plan was to populate the manually selected parameters list by just grabbing all domain types in the task graph (upstream from the target outputs).

Then please reread my original comment on why that is not a solution: https://github.com/scipp/essreduce/issues/74#issuecomment-2286095005. If you just grad "all domain types" you loose this self-documenting/self-explanatory part of the UI (and are left with something that is little better than just using Python to set parameters on a workflow).

jokasimr commented 1 month ago

This is getting pretty confused so I think it's better we talk directly.

As I see it this issue is not about how to populate the parameters list. It's about having a multiple -select instead of a checkbox next to each (selectable) parameter in the form.

I was thinking we just to have one big list, but really, how to determine what parameters are selectable or not seems orthogonal to the question in the issue.

SimonHeybrock commented 1 month ago

Notes from discussion:

Example: