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

Add nested select #5791

Merged
merged 19 commits into from
Nov 30, 2023
Merged

Add nested select #5791

merged 19 commits into from
Nov 30, 2023

Conversation

ahuang11
Copy link
Contributor

@ahuang11 ahuang11 commented Oct 31, 2023

import panel as pn
import param

pn.extension()

d = {
    "Andrew": {
        "temp": [1000, 925, 700, 500, 300],
        "vorticity": [500, 300],
    },
    "Ben": {
        "temp": [500, 300],
        "windspeed": [700, 500, 300],
    },
}

from panel.widgets import NestedSelect

NestedSelect(options=d, level_names=["product", "variable", "level"]).show()

Have selects depend on each other; worked on this together with @benbarn313.

Screen.Recording.2023-10-31.at.3.27.24.PM.mov
  • tests
  • docs

Wondering if we should support this use case, where the nested levels are not even.

d = {
    "Andrew": {
        "temp": [1000, 925, 700, 500, 300],
        "vorticity": [500, 300],
    },
    "Ben": {
        ["temp", "windspeed"],
    },
}

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (d24bf1c) 82.68% compared to head (d5f35e0) 84.33%.

Files Patch % Lines
panel/widgets/select.py 94.85% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5791      +/-   ##
==========================================
+ Coverage   82.68%   84.33%   +1.64%     
==========================================
  Files         291      291              
  Lines       43071    43346     +275     
==========================================
+ Hits        35615    36554     +939     
+ Misses       7456     6792     -664     
Flag Coverage Δ
ui-tests 40.88% <13.09%> (+2.26%) ⬆️
unitexamples-tests 72.39% <97.45%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ahuang11
Copy link
Contributor Author

ahuang11 commented Nov 4, 2023

I believe this is ready to add tests + docs

Screen.Recording.2023-11-03.at.7.32.07.PM.mov

@ahuang11
Copy link
Contributor Author

ahuang11 commented Nov 13, 2023

import param
import panel as pn
from panel.widgets.select import *

pn.extension()
options = {
    "Ben": ["A", "B"],
    "Andrew": {
        "temp": [1000, 925, 700, 500, 300],
        "vorticity": [500, 300],
    },
}
select = NestedSelect(options=options, value=["Ben", "B"])
select

I added variable depth support

Screen.Recording.2023-11-13.at.7.33.23.AM.mov

@ahuang11
Copy link
Contributor Author

ahuang11 commented Nov 15, 2023

Talked with Philipp. We decided to first rollback variable level nesting, e.g. the following needs more design and should not be allowed for now due to its complexity.

options = {
    "Ben": ["A", "B"],
    "Andrew": {
        "temp": [1000, 925, 700, 500, 300],
        "vorticity": [500, 300],
    },
}
select = NestedSelect(options=options, value=["Ben", "B"])

However, given uniform level nesting, we're still debating whether value should be a tuple vs dict, e.g. given:

options = {
    "Andrew": {
        "temp": [1000, 925, 700, 500, 300],
        "vorticity": [500, 300],
    },
    "Ben": {
        "temp": [500, 300],
        "windspeed": [700, 500, 300],
    },
}

levels = [{'name': 'Name', 'type': MultiChoice, 'default': 'Ben'}, {'name': 'Variable', 'default': "temp"}, {'name': 'Pressure', 'type': DiscreteSlider, 'default': 300}]

select = NestedSelect(options=options, levels=levels)

Should select.value == ("Ben", "temp", 300) or select.value == {"Name": "Ben", "Variable": "temp", "Pressure": 300}?

Pros of tuple is that levels names is not required; it's by index. However, with many nesting of levels, it can be complicated to extract the desired output value (e.g. if I need to search for Pressure level, I need to know it's index 2 and Variable is index 1)

Pros of dict is that it's more explicit and easier to extract the desired value, e.g. value["Variable"]. The downside is that levels names must be specified, or else the dict keys are just enumerations of the levels, e.g. {0: "Ben", 1: "temp", 2: 300}. I am leaning towards dict.

@jbednar
Copy link
Member

jbednar commented Nov 16, 2023

It seems to me that variable nested levels will come up often, particularly during development. The dict format sounds better to me. Is there any future for dynamic updating with this approach? E.g. instead of a list for a particular entry can you imagine accepting a callable? If so, what would the arguments to the callable be? It's fine to say that more complex cases like that should be handled by nested Parameterized classes; just seeing if they are explicitly out of scope or only not yet considered.

ahuang11 and others added 4 commits November 27, 2023 10:05
#5925)

* completed tests for nested select. modified code to handle test cases.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fixed test cases

* slight changes

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@ahuang11
Copy link
Contributor Author

ahuang11 commented Nov 27, 2023

instead of a list for a particular entry can you imagine accepting a callable

Good point! I think it might be out of scope for now, but I imagine it can be a feature enhancement in the near future without breaking changes, when we think of the design a little more...

Here's an example:

options = {
    "Andrew": {
        "temp": get_temp_levels,
        "windspeed": get_wspd_levels,
    },
    "Ben": {
        "temp": get_temp_levels,
        "windspeed": get_wspd_levels,
    },
}

However, I don't know the input kwargs either. Perhaps it's just the current nested select value

@ahuang11 ahuang11 marked this pull request as ready for review November 27, 2023 23:00
@philippjfr
Copy link
Member

It seems to me that variable nested levels will come up often, particularly during development.

Can we gather some use cases for this? I wasn't able to come up with many good ones.

@jbednar
Copy link
Member

jbednar commented Nov 28, 2023

During development, if you have to add the same number of items to every sublevel, it seems tricky to try things out...

@philippjfr
Copy link
Member

Variable nesting refers to the number of levels not the number of items at each level of I understand things correctly.

@ahuang11
Copy link
Contributor Author

Variable nesting refers to the number of levels not the number of items at each level of I understand things correctly.

That is correct.

@jbednar
Copy link
Member

jbednar commented Nov 28, 2023

I think we're all saying the same thing, but to be specific, I'm referring to filling out suboptions here for one option but not yet the others:

  • Category A:
    • Option 1
    • Option 2
      • Suboption 1
      • Suboption 2

The question is whether Option 1 is required to have suboptions here once you add them for Option 2. I believe the answer is yes right now, but ideally we wouldn't have to add them at every level if we add them at one level.

@ahuang11
Copy link
Contributor Author

The question is whether Option 1 is required to have suboptions here once you add them for Option 2. I believe the answer is yes right now

That is correct. Maybe in a future PR when callbacks are supported, we can also have dynamic variable nesting as well.

@philippjfr
Copy link
Member

I think we're all saying the same thing,

I don't think we are (or maybe I'm still misunderstanding) but I think I get your request now. Effectively you want a partial specification to be valid so you don't have to fill out the full tree immediately just to get something working. That's not the same as having a variable number of levels though. I fully agree that what you're saying should be supported but we should make a decision what the behavior of such a partial specification should be, i.e. levels that are not fully specified should either:

  • Display an empty select widget
  • Hide the widget for a level if no options are provided

@philippjfr
Copy link
Member

So to be clear, I think we can and should allow what you described without actually supporting variable levels of nesting as I understand it based on Andrew's initial description of it.

@ahuang11
Copy link
Contributor Author

ahuang11 commented Nov 28, 2023

Ok, partial definitions of options is now possible and the subsequent widgets that have no selections are hidden:

select = pn.widgets.NestedSelect(
    options={
        "NAM": {},
        "GFS": {
            "0.25 deg": ["00Z", "06Z", "12Z", "18Z"],
            "0.5 deg": ["00Z", "12Z"],
            "1 deg": ["00Z", "12Z"],
        },
    },
    levels=["Model", "Resolution", "Initialization"],
)
select
image image

The first select widget is always visible (unless user sets visible=False manually in levels).

Screen.Recording.2023-11-28.at.12.56.37.PM.mov

Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

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

Looks great!

@philippjfr philippjfr merged commit 905cf91 into main Nov 30, 2023
12 of 13 checks passed
@philippjfr philippjfr deleted the add_nested_select branch November 30, 2023 08:53
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

Successfully merging this pull request may close these issues.

None yet

4 participants