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

Improve Switchable Widget and handle intermediate parameters #74

Open
Tracked by #61
YooSunYoung opened this issue Aug 13, 2024 · 16 comments
Open
Tracked by #61

Improve Switchable Widget and handle intermediate parameters #74

YooSunYoung opened this issue Aug 13, 2024 · 16 comments

Comments

@YooSunYoung
Copy link
Member

YooSunYoung commented Aug 13, 2024

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

This was referenced Aug 13, 2024
@YooSunYoung YooSunYoung changed the title Improve Switchable Widget. Improve Switchable Widget and handle intermediate parameters Aug 13, 2024
@jokasimr
Copy link
Contributor

jokasimr commented Aug 13, 2024

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

jokasimr commented Aug 13, 2024

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
0. User selects sans workflow and IofQ output.

  1. User opens parameters multiple-select.
  2. User selects "DirectBeam"
  3. User presses "Generate parameters"
    -> Parameter form is generated, the parameter form contains the direct beam widget.

@YooSunYoung
Copy link
Member Author

YooSunYoung commented Aug 13, 2024

  • Remove radio button and use check box + stack for optional widget instead
    Currently we are using that combination for the switchable, but we can use them for optional if we use different solution.
    Then we don't have to heck the style for radio box to make it cleaner.

@SimonHeybrock
Copy link
Member

Ok, so we keep the Parameter.switchable field 👍

@jokasimr
Copy link
Contributor

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
Copy link
Member

SimonHeybrock commented Aug 13, 2024

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
Copy link
Contributor

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
Copy link
Contributor

jokasimr commented Aug 13, 2024

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
Copy link
Member

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
Copy link
Contributor

jokasimr commented Aug 14, 2024

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
Copy link
Member

SimonHeybrock commented Aug 14, 2024

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: #74 (comment). 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
Copy link
Contributor

jokasimr commented Aug 14, 2024

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
Copy link
Member

SimonHeybrock commented Aug 14, 2024

Notes from discussion:

  • typical_outputs = (...)
  • alternative_inputs = (...) (not including the usual inputs, i.e., root nodes, previously identified as Parameter.switchable)
  • typical_outputs -> prune graph
  • workflow defines params
  • user selects alternative inputs:
    • Add widget for that input
    • Remove all widgets of now unused params (that only lead to the alternative input but nowhere else)

Example:

  • Base graph A->B->C->D
  • Typical outputs: C, D
  • Alternative inputs: B
  • If user does not select alternative input, widget based on parameter for A will be generated.
  • If user selects the alternative input B, the widget for A will not be generated, instead widget for B is added.

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

No branches or pull requests

3 participants