holoviz / param

Param: Make your Python code clearer and more reliable by declaring Parameters
https://param.holoviz.org
BSD 3-Clause "New" or "Revised" License
412 stars 69 forks source link

`ListSelector`: regression when `check_on_set=False` #807

Closed maximlt closed 10 months ago

maximlt commented 11 months ago

I think this was caused by https://github.com/holoviz/param/pull/794:

import param

ls = param.ListSelector([1, 2], objects=[1, 2, 3], check_on_set=False)
ls.objects
# [1, 2, 3, [1, 2]]  :(
jbednar commented 11 months ago

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 commented 11 months ago

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_onset 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