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

ListSelector: regression when check_on_set=False #807

Closed
maximlt opened this issue Jul 31, 2023 · 2 comments · Fixed by #817
Closed

ListSelector: regression when check_on_set=False #807

maximlt opened this issue Jul 31, 2023 · 2 comments · Fixed by #817
Labels
type-bug Bug report
Milestone

Comments

@maximlt
Copy link
Member

maximlt commented Jul 31, 2023

I think this was caused by #794:

import param

ls = param.ListSelector([1, 2], objects=[1, 2, 3], check_on_set=False)
ls.objects
# [1, 2, 3, [1, 2]]  :(
@maximlt maximlt added the type-bug Bug report label Jul 31, 2023
@maximlt maximlt added this to the 2.0 milestone Jul 31, 2023
@jbednar
Copy link
Member

jbednar commented Aug 3, 2023

Hmm; I think you're right. I think this could be fixed by adding an _update_state to ListSelector:

    def _update_state(self):
        if self.check_on_set is False and self.default is not None:
            for o in self.default:
                if o not in self.objects:
                    self.objects.append(o)

Still, now that I look more closely at the surrounding code, I'm wondering whether _update_state doesn't just duplicate _ensure_value_is_in_objects, with the original problem being the logic by which _ensure_value_is_in_objects is invoked. I'm inclined to back #794 out and then (a) let _validate get called whether or not check_on_set is true, so that it invokes _ensure_value_is_in_objects, and (b) implement _ensure_value_is_in_objects for ListSelector as essentially the above code.

As best I can tell, _ensure_value_is_in_objects is already called at the right spot in the code, i.e. after things are defined, but I could be missing something.

@maximlt
Copy link
Member Author

maximlt commented Aug 3, 2023

Actually I'm even thinking that #693 is a silly usage of Param and I'm no longer sure we should "fix" it. That's the example I shared in #693, why would you not include 3 in the objects?? On top of that check_on_set is pretty clearly worded with on_set and not on_init.

import param

class P(param.Parameterized):
    s = param.Selector(default=3, objects=[1, 2], check_on_set=False)

p = P()

print(p.s, p.param.s.objects)
# 3 [1, 2]

So yes feel free to revert #794! If you still want that change and have another implementation that doesn't require _update_state please go for it. I've added it as I found the Selector Parameter(s) to be a pain to get right in an inheritance context as it has a few attributes computed dynamically.


A use case I'd like to make sure works well between Param and Panel is when a Selector is instantiated without its objects attribute being defined yet as it needs to be populated later on (e.g. from data coming from a database) and when check_on_set needs to be False to allow the user to extend the list of selected options (restrict=False mode of the Bokeh autocomplete input widget). I care about this particular use case as I have it in place many times in a big app I maintain.

import pandas as pd
import param
import panel as pn
pn.extension()

class P(pn.viewable.Viewer):

    data = param.DataFrame(precedence=-1)
    s = param.Selector(check_on_set=False)

    def __init__(self, **params):
        super().__init__(**params)
        if self.data is not None and not self.data.empty:
            objs = sorted(self.data['names'].unique())
            self.param.s.objects = objs
            self.s = ''

    def __panel__(self):
        return pn.widgets.AutocompleteInput.from_param(
            self.param.s,
            restrict=False,  # check_on_set should probably be mapped to restrict
            min_characters=1
        )

data = pd.DataFrame(dict(names=['bob', 'bill', 'alice', 'julia']))
# data = data.iloc[:0, :]

p = P(data=data)
p

Usually I don't want the default value to be populated, I want the user to enter their own value (to check that they've indeed entered something in that field when validating a form that has more widgets than this one), which is why I set self.s = ''. It effectively makes '' a valid option but that seems okay.


@philippjfr #794 broke this Panel test as #794 adds the default value in objects on Parameter instantiation so it's also in autocomplete.completions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants