thomasdeneux / paramqt

Qt interface for param
BSD 3-Clause "New" or "Revised" License
2 stars 0 forks source link

Parameter subobjects #1

Open jbednar opened 4 years ago

jbednar commented 4 years ago

Given this code that's similar to https://github.com/thomasdeneux/param_qt/blob/master/test_ParamQt.py#L98:

class PosPar(pm.Parameterized):

        shape = GObjectSelector('star',
                                ['circle', 'polygon', 'star'],
                                style='button',
                                doc="select drawing's shape",
                                allow_None=True)
        ...

    class EdgePar(pm.Parameterized):
        color = GColor('#000000', allow_None=True)
        ...

    class FillPar(pm.Parameterized):
        color = GColor('#008080', allow_None=True)
        ...

    class ShapePar(pm.Parameterized):
        antialiasing = GBoolean(True, expert=True)
        pos = PosPar
        edge = EdgePar
        fill = FillPar

You reported in https://github.com/holoviz/param/issues/353 that "if class ShapePar above is instantiated, its pos, edge, fill subparameters are shared between instances". I'm surprised that it works at all as you have written; I wouldn't have thought to put the actual Parameter class into a Parameterized. Parameters were designed to be instantiated in the Parameterized class, so that you would use pos = PosPar() in ShapePar, and I'm not surprised that you'd see strangely shared behavior if you don't instantiate them. (What's surprising to me is that it functions at all when used that way!).

Can you report what happens when you use pos = PosPar(), etc. here as normally done? Let's investigate any problems you find with that approach before trying to trace through the other not-at-all-supported way.

thomasdeneux commented 4 years ago

Thank you for looking at param_qt, let's try to keep the momentum and clear potential issues.

After i posted on holoviz/param#353 the message you copied here, i have improved ParamQt.py. Now i have almost no more problems, but you might dislike some specific choices that i made and it is the time to discuss them, before going further.

But before all i believe it would be nice that you execute test_ParamQt.py to get a sense of what i have developed so far, and understand the automatic behaviors for translation, expert parameters, parameters depending on others, etc.

This was a first general comment, now i go into your question.

thomasdeneux commented 4 years ago

As said above, i solved my earlier problem "if class ShapePar above is instantiated, its pos, edge, fill subparameters are shared between instances". param_qt can be used the 2 following ways:

1) If you wish to instantiate GParameterized classes (my subclass of `pm.Parameterized'), which is what you recommend, you write:

class FillPar(GParameterized):
    start_folded=True

    color = GColor('#008080', allow_None=True)
    fill_rule = GObjectSelector(Qt.OddEvenFill,
                                objects={'full': Qt.WindingFill,
                                         'odd-even': Qt.OddEvenFill},
                                visible=['color', expert_p])

class ShapePar(GParameterized):
    antialiasing = GBoolean(True, visible=expert_p)

    def __init__(self):
        super(ShapePar, self).__init__()
        self.fill = FillPar()

and use it that way:

2) But it is also possible to create control panel on the class itself. In this case you should write:

class FillPar(GParameterized):
    start_folded=True

    color = GColor('#008080', allow_None=True)
    fill_rule = GObjectSelector(Qt.OddEvenFill,
                                objects={'full': Qt.WindingFill,
                                         'odd-even': Qt.OddEvenFill},
                                visible=['color', expert_p])

class ShapePar(GParameterized):
    antialiasing = GBoolean(True, visible=expert_p)

    fill = FillPar

and then:

This usage, as you can see, offers a simple syntax, and might be preferred when one manipulates only a single set of parameter values in the whole program.

Note that test_ParamQt.py allows you to test both usages by setting TEST_INSTANCE to True or False in the first lines.

thomasdeneux commented 4 years ago

So in addition to the general feedback here are my more focused questions:

1) do the two syntaxes exposed above for nested parameter structures make sense to you?

2) you will see in ParamQt.py (line 164) that i needed to sub-class Parameters (the original question with which my discussion with you started!), used precedence rather as an addition storage for more attributes such as "style", and added visible and enabled slots (slots are needed to take advantage of param's watching mechanism when their value will be changed). This is not fully satisfying, but would the param team accept to add 3 slots user, visible and enabled?! (note that enabled plays more or less the role of a "not readonly")

3) I had a usage of @pm.depends(str) that did not work properly when not instantiating GParameterized class; but i now understand from you that best practice is not instantiate pm.Parameterized or their sub-classes, so this issue becomes quite minor and can be kept for later

jbednar commented 4 years ago

execute test_ParamQt.py

The code isn't currently runnable, because ParamQT.py contains a line "from GUI.FlowLayout import FlowLayout", and there's no "GUI" module. FlowLayout is used in several places, so it doesn't look like it will run without that.

jbednar commented 4 years ago
  1. do the two syntaxes exposed above for nested parameter structures make sense to you?

Without having runnable code that I could try out different ways, I could be confused, but I don't think I recommend any of the above approaches. What I would expect is something like:

class FillPar(GParameterized):
    start_folded= GBoolean(True)
    color = GColor('#008080', allow_None=True)
    fill_rule = GObjectSelector(Qt.OddEvenFill,
                                objects={'full': Qt.WindingFill,
                                         'odd-even': Qt.OddEvenFill},
                                visible=['color', expert_p])

class ShapePar(GParameterized):
    antialiasing = GBoolean(True, visible=expert_p)
    fill = GParameter(FillPar())

ShapePar.fill.color = '#ff00ff'

p = ShapePar()
p.fill.color = '#ff0000'

I.e., defining a parameter fill whose default value is some Parameterized object. Once you've done so, you can set fill.color either at the class level (ShapePar.fill.color) or at the instance level (p.fill.color). In this case both ways to set it will have roughly the same effect, because all instances will share that same FillPar object instance by default, but you can define fill = GParameter(FillPar(),instantiate=True) if you want every instance of ShapePar to have its own independent fill object cloned for that instance. Here instantiate should perhaps have been called clone_parameter_value_per_parameterized_instance, because that's what it does; not sure whether you need that here.

  1. you will see in ParamQt.py (line 164) that i needed to sub-class Parameters (the original question with which my discussion with you started!), used precedence rather as an addition storage for more attributes such as "style", and added visible and enabled slots (slots are needed to take advantage of param's watching mechanism when their value will be changed). This is not fully satisfying, but would the param team accept to add 3 slots user, visible and enabled?! (note that enabled plays more or less the role of a "not readonly")

In general, we're happy to make changes to Param to support what you're doing; we just first need to make sure there's not a better way that's already supported.

  1. I had a usage of @pm.depends(str) that did not work properly when not instantiating GParameterized class; but i now understand from you that best practice is not instantiate pm.Parameterized or their sub-classes, so this issue becomes quite minor and can be kept for later

I'm not arguing against instantiating Parameterized subclasses; on the contrary, that's normally what one would do, particularly if the same set of configuration options can be reused in multiple locations and makes sense to have different values in each. I'm not sure what recommendation you're referring to, but I was probably objecting to a particular usage and not to the general notion of instantiation. In any case, just raise the specific issue you encountered and we can go from there!

thomasdeneux commented 4 years ago

Hello @jbednar, really sorry about the code not be runnable, i had copy-pasted it without even testing! This is fixed now.

thomasdeneux commented 4 years ago
  1. do the two syntaxes exposed above for nested parameter structures make sense to you?

... but you can define fill = GParameter(FillPar(),instantiate=True) if you want every instance of ShapePar to have its own independent fill object cloned for that instance. Here instantiate should perhaps have been called clone_parameter_value_per_parameterized_instance, because that's what it does; not sure whether you need that here.

oh... this instantiate=True is indeed exactly what i was missing all this time!!! I will definitely do this.

  1. additional slots to avoid subclassing Parameter?

In general, we're happy to make changes to Param to support what you're doing; we just first need to make sure there's not a better way that's already supported.

* For "enabled", why not just use "readonly", if that's what it is?  I'm not sure why inverting the sense of it makes it achieve anything not already doable using "readonly".

* For "visible", can't you just use the existing precedence?  E.g. toggling visibility could flip whatever numerical value it has from positive to negative, and then anything over zero would be displayed.

* If "enabled" and "visible" are covered, then presumably you'd no longer need "user"?

* If you did have "user", wouldn't that one extra slot be sufficient, if it's a dictionary of key/value pairs, because you could then put in whatever you want?

It would be fine for me indeed to use the "readonly" slot for "enabled", "precedence" for "visible".

And to create "user" for additional key/value pairs that do not need the watching mechanism (i for example use a "style" value, for example to choose between slider or text in the case of a numerical parameter).

But i realize now that adding slots is not enough to avoid subclassing Parameter. There is also all the new syntax to make the visibility or enabling of a parameter depend on the value of some other parameter: see my code of _GraphicParameter and the example usage in test_ParamQt.py. If i want to avoid subclassing Parameter, this code should be included directly into param, this would require a lot of coordination with you, this is not necessarily ideal, what do you say?

  1. I had a usage of @pm.depends(str) that did not work properly when not instantiating GParameterized class; but i now understand from you that best practice is not instantiate pm.Parameterized or their sub-classes, so this issue becomes quite minor and can be kept for later

I'm not arguing against instantiating Parameterized subclasses; on the contrary, that's normally what one would do, particularly if the same set of configuration options can be reused in multiple locations and makes sense to have different values in each. I'm not sure what recommendation you're referring to, but I was probably objecting to a particular usage and not to the general notion of instantiation. In any case, just raise the specific issue you encountered and we can go from there!

Sorry again, a typo!! I meant " best practice is TO instantiate pm.Parameterized or their sub-classes", while the bug i encountered was when not instantiating. I have to do some work to get back the bug: i keep this for later and if needed will open a param issue or PR for it.

jbednar commented 4 years ago

Ok, test_ParamQt.py works now! Thanks for updating it. I'd forgotten how nice it is to work with git when the repo is small and there's no history; it's so fast!

And the code example is very nice and very fast as well. I really like how you are using Param with Qt here; exactly how we had been envisioning it back when we were stuck with Tk's older versions (when it was really clunky) and wishing we were on Qt. Tk 8.5 finally improved Tk, and Qt still had licensing issues at the time, so we never did make the jump to Qt, but it's nice to see how it would all work now if we need proper OS-native GUIs at some point in the future. The example in that test is actually almost identical to our Tk-based interface to ImaGen, though it's been quite some time since we used that.

best practice is TO instantiate pm.Parameterized or their sub-classes

Ok, makes sense. Just to be clear, we are happy for people to have GUIs that manipulate either the class Parameter objects or a set of instantiated objects; each approach has its uses. E.g. one often edits at the class level for configuring the overall preferences before starting a workflow, then edits at the instance level for working with a particular simulation or control run. It's all good!

But I realize now that adding slots is not enough to avoid subclassing Parameter. There is also all the new syntax to make the visibility or enabling of a parameter depend on the value of some other parameter: see my code of _GraphicParameter and the example usage in test_ParamQt.py. If i want to avoid subclassing Parameter, this code should be included directly into param, this would require a lot of coordination with you, this is not necessarily ideal, what do you say?

It's worth reading https://panel.holoviz.org/user_guide/Param.html even though you aren't using Panel, just to see how we're approaching making a GUI for param. In this case, when working with Panel we use @param.depends and precedence for controlling visibility. Here's an example, where you can see that at the user level the parameter handling in the Sine class is independent of Panel, but if you run it you can see that Panel respects the precedence and hides the "phase" widget if the user drags the "frequency" widget past 1.5:

import param, panel as pn, numpy as np
pn.extension()

def param_visible(param, condition):
    sign = -1.0 if condition else 1.0
    if param.precedence is None:
        param.precedence = sign
    else:
        param.precedence = sign * abs(param.precedence)

class Sine(param.Parameterized):

    phase = param.Number(default=0, bounds=(0, np.pi))

    frequency = param.Number(default=1, bounds=(0.1, 2))

    @param.depends('frequency', watch=True)
    def update_visibility(self):
        param_visible(self.param.phase, self.frequency>1.5)

    @param.depends('phase', 'frequency')
    def view(self):
        y = np.sin(np.linspace(0, np.pi * 3, 40) * self.frequency + self.phase)
        y = ((y - y.min()) / y.ptp()) * 20
        array = np.array(
            [list((' ' * (int(round(d)) - 1) + '*').ljust(20)) for d in y])
        return pn.pane.Str('\n'.join([''.join(r) for r in array.T]), height=380, width=500)

sine = Sine(name='ASCII Sine Wave')
pn.Row(sine.param, sine.view)

It would be great if ParamQt could respect the precedence in the same way, so that you wouldn't need any separate visibility code and the same user-level code could work with both ParamQt and with Panel (at the parameter level, at least).

thomasdeneux commented 4 years ago

Hello, glad that you like it,

I understand how precedence is used to control the visibility upon other parameter values, through a @param.depends decoratedfunction (and we could do the same for enabling with readonly).

In fact i started doing this way, but later on i introduced a lighter syntax where the minimum 3-lines function is replaced by a mere additional keyword in the parameter definition (visible=..., see example below).

class EdgePar(GParameterized):
    color = GColor('#000000', allow_None=True)
    width = GNumber(1., bounds=[0, 20], style='slider', mode='left',
                    visible='color')  # -> visible if bool(color) == True, i.e. if color is not None
    join_style = GObjectSelector(QtGui.QPen().joinStyle(),
                                 objects={'bevel': Qt.BevelJoin,
                                          'miter': Qt.MiterJoin,
                                          'round': Qt.RoundJoin},
                                 visible=['color', expert_p])  # -> visible if color is not None and parameter expert_p is True
    miter_limit = GNumber(math.inf,  
                          style='edit',
                          visible=['color', ('join_style', Qt.MiterJoin)])  # -> visible if color is not None and join_style is Qt.MiterJoin

Note that the actual call to Parameters.watch for watching the dependency (or dependencies) occurs upon setting of the owner attribute of the _GraphicParameter object.

This syntax becomes really useful when building large structures with many parameters, but i need to subclass pm.Parameter to introduce it!

jbednar commented 4 years ago

Ok, I see it now. Sorry for being dense. What it boils down to is not that you are unable to express visibility using the existing mechanisms, but that you want a compact declarative syntax for doing so. That's a feature request to file on Param, namely to propose a concise syntax for expressing that a given parameter's visibility should be gated on some other parameter being non-None (and/or non-empty, for sequences) or on that other parameter having a specified value.

I think that's a reasonable request and I like your syntax; it does seem useful (for Panel as well as ParamQt). My only concern is that Param is fundamentally not a GUI library, and so we do our best to ensure that declarative specifications in Param are expressed in a way that makes sense with or without a GUI. E.g. precedence can determine the order that Parameters are listed in, and whether a given Parameter is included by default, with or without a GUI. I guess visible could work the same way, and we'd say that it controls whether the parameter is visible in a user-level display of the current parameter values, so that some sub-parameters can be suppressed in such a listing by declaring their visibility settings. It would be nice if the visible slot could also be set to a callable for full generality when needed, though I'm not sure what would trigger that callable to be executed.

In any case, please outline this proposal on an issue in Param and we can get it going towards a PR. Thanks!

thomasdeneux commented 4 years ago

Great, i am also favorable to have this new syntax already in param.

There is probably still room for some debate, but in general it seems that my main questions are answered now.

I'll work on the different tasks mentioned here in the coming days/weeks, including opening the issue in Param. See you.

jbednar commented 4 years ago

BTW, if the request were just to be able to support an expert meta-parameter as you have here, that's something that I think is already reasonably well supported. I.e., just assign precedence=Expert, precedence=Debugging, etc., for suitable parameters, with those values having a lower numeric value than the default precedence, then set up a method watching an expert parameter and setting a display_threshold to either higher or lower than the value of Expert. But it's the usage for color and join_style illustrated above where I think the visible approach is a big win over what we have now.

thomasdeneux commented 4 years ago

Hello @jbednar, i am working on point 1. above: use a more standard syntax for parameter subobjects, which is also the title of the present issue. (quick reminder for the two other points: 2 -> we agreed to add a slot 'user' to param, and to include the syntax for automatic updates of visibility and readonly into param; 3 -> we'll see later).

So i am confronted with an issue: here is a simplified section of my previous code, in the case where we want to instantiate GParameterized objects.

# general parameters
class GeneralPar(GParameterized):
    show_expert = GBoolean(True, label='Show expert parameters')
    ...

# instantiate
general_par = GeneralPar()

# shape parameters: visibility of some parameters will depend on general_par.show_expert value
class PosPar(GParameterized):
    ...
    x = GNumber(0., bounds=[-1, 1], visible=general_par.param['show_expert'])
    ...

# all parameters are gathered in a single GParameterized object
class AllPar(GParameterized):

    def __init__(self):
        super(AllPar, self).__init__()
        self.general = general_par
        self.shape = ShapePar()

# instantiate
all_par = AllPar()

I agree that it would be much nicer to define

class AllPar(GParameterized):

    general = GParameter(GeneralPar(), instantiate=True)
    shape = GParameter(ShapePar(), instantiate=True)

and we have the same code regardless of whether we want to instantiate AllPar or not before controling it from a GUI. But it seems impossible to write the visibility dependence for 'x' parameter that would work in all cases! The only syntax that i see is something with relative paths as below, but this is not so compelling, nor will it be easy to implement.

# general parameters
class GeneralPar(GParameterized):
    show_expert = GBoolean(True, label='Show expert parameters')
    ...

# shape parameters: visibility of some parameters will depend on general_par.show_expert value
class PosPar(GParameterized):
    ...
    x = GNumber(0., bounds=[-1, 1], visible='..general.show_expert')
    ...

# all parameters are gathered in a single GParameterized object
class AllPar(GParameterized):

    general = GParameter(GeneralPar(), instantiate=True)
    shape = GParameter(ShapePar(), instantiate=True)

# instantiate
all_par = AllPar()
jbednar commented 4 years ago

Generally we use inheritance in such cases:

# general parameters
class GeneralPar(GParameterized):
    show_expert = GBoolean(True, label='Show expert parameters')
    ...

# pos parameters: visibility of some parameters will depend on general_par.show_expert value
class PosPar(GeneralPar):
    ...
    x = GNumber(0., bounds=[-1, 1], visible='show_expert')
    ...

# all parameters are gathered in a single GParameterized object
class AllPar(GParameterized):

    shape = GParameter(ShapePar(), instantiate=True)
    pos = GParameter(PosPar(), instantiate=True)

# instantiate
all_par = AllPar()

But that will behave differently from your current app, because show_expert will be visible in both the shape and the pos subobjects, not as a separate list of general parameters, though you can always hide inherited parameters by declaring them with a lower precedence than they have in the parent class.

Anyway, what you are trying to do here is some sort of global parameters respected from everywhere, which is fine, but does require you to either implement such code yourself in your GUI code, or to have a way to refer to such global parameters from your individual objects. If you wanted a syntax for making global references, it could e.g. be visible=(GeneralPar,'show_expert') (a tuple of a class or instance followed by a parameter name, or visible=GeneralPar.param.show_expert (a direct reference to the parameter object you're tracking).

Instead of any of these things, I'd still suggest simply using the precedence, which is already globally controllable and thus has the property you're after. I.e., sure, implement visible for your other use cases, but also respect precedence, and then declare the precedence of your expert parameters appropriately. That way you can easily put up a show_expert button anywhere and put a method watching it that adjust the global display_threshold appropriately. The visible property seems useful within a tight inheritance/object hierarchy, and I don't think it's necessary to make it so powerful that it can reference parameters on arbitrary external objects.

thomasdeneux commented 4 years ago

Generally we use inheritance in such cases:

But that will behave differently from your current app, because show_expert will be visible in both the shape and the pos subobjects, not as a separate list of general parameters, though you can always hide inherited parameters by declaring them with a lower precedence than they have in the parent class.

Indeed I would not favor this solution.

Anyway, what you are trying to do here is some sort of global parameters respected from everywhere, which is fine, but does require you to either implement such code yourself in your GUI code, or to have a way to refer to such global parameters from your individual objects. If you wanted a syntax for making global references, it could e.g. be visible=(GeneralPar,'show_expert') (a tuple of a class or instance followed by a parameter name, or visible=GeneralPar.param.show_expert (a direct reference to the parameter object you're tracking).

Exactly: this is what appears in the first code block of my previous comment: i first define the 'global parameter' general_par, and then use it with visible=general_par.param['show_expert'].

My question was how to write a code that works both if i want to build GUI on instantiated GParameterized objects or rather on GParameterized classes. My conclusion is that there will rather be two different writings.

If we work with instances:

class GeneralPar(GParameterized):
    show_expert = GBoolean(True, label='Show expert parameters')

general_par = GeneralPar()  # instantiate

class PosPar(GeneralPar):
    x = GNumber(0., bounds=[-1, 1], visible=general_par.param.show_expert)

class AllPar(GParameterized):
    general = GParameter(general_par)
    pos = GParameter(PosPar())  # no need to put instantiate=True as long as we won't use the class-level parameters

all_par = AllPar()  # instantiate

gui = ControlPanel(all_par)  # build the GUI

And if we work at the class level:

class GeneralPar(GParameterized):
    show_expert = GBoolean(True, label='Show expert parameters')

class PosPar(GeneralPar):
    x = GNumber(0., bounds=[-1, 1], visible=GeneralPar.param.show_expert)

class AllPar(GParameterized):
    general = GParameter(GeneralPar)
    pos = GParameter(PosPar)

gui = ControlPanel(AllPar)  # build the GUI

I find it more natural to work with GParameterized instances than classes, but the code is the prettiest when working with classes!

Instead of any of these things, I'd still suggest simply using the precedence, which is already globally controllable and thus has the property you're after. I.e., sure, implement visible for your other use cases, but also respect precedence, and then declare the precedence of your expert parameters appropriately. That way you can easily put up a show_expert button anywhere and put a method watching it that adjust the global display_threshold appropriately. The visible property seems useful within a tight inheritance/object hierarchy, and I don't think it's necessary to make it so powerful that it can reference parameters on arbitrary external objects.

Do you mean to use precedence to control visibility based on some level of expertise, and use an additional slot visible to additionally control visibility based on other parameters? I had understood so far that we would not create a visible slot but only create an alias to precedence.

Let's take an example from my real application: i have for example a property with complex visibility rule visible=(_p_display_mode, [_modes.Expert, _modes.Developer]), (_p_hardware, 'AlphaBot'), ('algorithm', ['qlearning', 'DQN'])] (where _p_display_mode and _p_hardware are what you call 'global parameters').

It could be split into precedence=_modes.Expert (_modes is an enum, so this would mean display when display_mode or display_threshold is at least _modes.Expert) and visible=[(_p_hardware, 'AlphaBot'), ('algorithm', ['qlearning', 'DQN'])]. Why not, but you seemed to prefere not to create an additional slot, isn't it?

jbednar commented 4 years ago

Do you mean to use precedence to control visibility based on some level of expertise, and use an additional slot visible to additionally control visibility based on other parameters?

Right. Precedence needs to continue to be supported no matter what, as that's already offered by Param, and so no matter how visible is implemented it will need to interact with precedence in some way. I'd suggest making it an AND operation, so that only those that are visible and have an appropriate precedence are shown. visible would be controlling the domain-specific logic (in what circumstances is this parameter valid to show), and precedence would be controlling the level of expertise involved (regardless of whether it is valid, I may not want to show it anyway simply because it's a tricky concept best hidden from normal users). Only if it's both visible and of the appropriate precedence would it be shown.

I had understood so far that we would not create a visible slot but only create an alias to precedence.

I can't quite see how such an alias would work; seems tricky! precedence is currently used not just for visibility but also for sorting, so that you can explicitly bump up some of the parameters to the front of the list, so I don't think you can take over what precedence does to implement visible. visible seems to require a new slot, to me. Your example from a real application seems to illustrate this nicely, separating the logic (in visible) from the perceived difficulty (in precedence).

thomasdeneux commented 4 years ago

ok, now i think i will be able to fill a PR to param with all these features we discussed included