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 docs around allow_refs feature #862

Merged
merged 3 commits into from
Oct 3, 2023
Merged

Add docs around allow_refs feature #862

merged 3 commits into from
Oct 3, 2023

Conversation

philippjfr
Copy link
Member

Documents allow_refs and nested_refs.

@philippjfr philippjfr requested review from droumis and maximlt October 2, 2023 13:49
Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

LGTM!

In addition to the few comments I left:

  • Did you think about adding allow_refs and nested_refs to the beginning of Parameters.ipynb where all the common Parameter slots are listed?
  • It would be nice to demonstrate that passing references allows to avoid writing callbacks. Also it'd be nice to see you can watch a Parameter that's been set from a reference, to see that updating the reference does execute the callback (I had to try to make sure :D!). Param docs probably need some refactoring!

doc/user_guide/Parameters.ipynb Outdated Show resolved Hide resolved
doc/user_guide/Parameters.ipynb Show resolved Hide resolved
"- Functions or methods annotated with `param.depends`\n",
"- Functions wrapped with `param.bind`\n",
"- Reactive expressions declared using `param.rx`\n",
"- Asynchronous generators\n",
Copy link
Member

Choose a reason for hiding this comment

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

Allowing bare asynchronous generators isn't going to cause problems with param.Callable? Is it the kind of case were users will have to set allow_refs=False and won't have the option between passing a normal async generator or a "reference async generator"?
Super edge-case I know, it's just to understand the consequences.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is going to cause problems if you enable allow_refs=True but for a callable there's rarely ever a good reason to do so.

Copy link
Member Author

@philippjfr philippjfr Oct 2, 2023

Choose a reason for hiding this comment

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

The deeper point though is that currently we don't have a way to allow some reference types and not others, which I guess is a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

The trick I originally used in Panel was that I would first check if a value would pass _validate before I attempted to resolve the reference, the issue there is that Dynamic parameters (including Number) allow functions/generators and you want to allow references even on param.Parameter types.

Copy link
Member

Choose a reason for hiding this comment

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

It is going to cause problems if you enable allow_refs=True but for a callable there's rarely ever a good reason to do so.

Is allow_refs set to True for pn.viewable.Viewer objects?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not currently, but Jim strongly suggested it should be.

doc/user_guide/Parameters.ipynb Outdated Show resolved Hide resolved
doc/user_guide/Parameters.ipynb Show resolved Hide resolved
@maximlt
Copy link
Member

maximlt commented Oct 2, 2023

Thanks for addressing my comments, feel free to merge!

@philippjfr
Copy link
Member Author

Haven't addressed your last comments. So will do that, merge and then cut an RC.

Copy link
Member

@droumis droumis left a comment

Choose a reason for hiding this comment

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

I left some comments and suggestions. Besides the nesting example, the rest was mostly about wordsmithing. Feel free to take or leave any of it and then merge.

Copy link
Member

Choose a reason for hiding this comment

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

  • The rest of this page is pretty mechanistic until this section. e.g. what is the mechanism of the syncing? what is happening to unsync when b2.p is set?
  • What happens if you try to do b2 = B(p=b1.param.p) if allow_refs was False? Why?
  • Is there a limit on how many references can be made to the same object? impact on performance?
  • Any guards against circular referencing?

Copy link
Member

Choose a reason for hiding this comment

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

Suggestion for new intro text:

Beyond the custom attribute access mechanisms of a single Parameter, Param can link together multiple Parameters. With the allow_refs option, a Parameter can act as a dynamic reference to another Parameter. This enables the values of two or more Parameters to stay in sync, allowing for reactive development. Any change to the referenced Parameter's value is automatically reflected in all Parameters that reference it.

Copy link
Member

Choose a reason for hiding this comment

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

Any guards against circular referencing?

@philippjfr what is your take on bi-directionally linking Parameters in Param? I'd guess there's a way for this to be implemented, if so let's open an issue to discuss the API? I'd love Param 2.1 to have it to be able to deprecate Panel's .link.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, would like to see the same.

Copy link
Member

Choose a reason for hiding this comment

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

Here's my suggestion for new intro text, including a more descriptive header:

Dynamically linking parameters

Building on the previous discussion about the dual roles of a Parameter - as both a value holder and a metadata container - let's explore how parameters can go a step further by acting as dynamic references to other parameters. When configured with allow_refs=True, a Parameter can serve as a live link to another Parameter, mirroring its current value. This capability enables more intricate relationships between parameters, allowing for automatic value synchronization and forming the basis for reactive programming.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how the example used to demonstrate nested_refs is an example of nesting; u1 and u2 don't contain any references...?

I would have expected something more like this:

class U(param.Parameterized):
    a = param.Number()

class V(param.Parameterized):
    b = param.Number(allow_refs=True)

class W(param.Parameterized):
    c = param.Number(allow_refs=True, nested_refs=True)

u = U(a=5)
v = V(b=u.param.a)
w = W(c=v.param.b)

Copy link
Member Author

@philippjfr philippjfr Oct 3, 2023

Choose a reason for hiding this comment

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

This is what demonstrates nesting, i.e. the references are nested inside some other container:

w = W(c=[u1.param.a, u2.param.a]) 

Copy link
Member Author

Choose a reason for hiding this comment

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

Chaining references like you have done doesn't require any special support.

@philippjfr philippjfr merged commit 3428cd4 into main Oct 3, 2023
10 checks passed
@philippjfr philippjfr deleted the ref_docs branch October 3, 2023 13:07
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.

4 participants