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

Param with bounds should default to lower bound #911

Open
ahuang11 opened this issue Mar 2, 2024 · 7 comments
Open

Param with bounds should default to lower bound #911

ahuang11 opened this issue Mar 2, 2024 · 7 comments
Labels
type-bug Bug report

Comments

@ahuang11
Copy link
Contributor

ahuang11 commented Mar 2, 2024

import param
class Test(param.Parameterized):

    fps = param.Integer(
        bounds=(1, None),
        doc="The number of frames per second to use for the output.",
    )


Test()

ValueError: Integer parameter 'fps' must be at least 1, not 0.

@ahuang11 ahuang11 added type-bug Bug report TRIAGE User-submitted issue that hasn't been triaged yet. labels Mar 2, 2024
@philippjfr philippjfr removed the TRIAGE User-submitted issue that hasn't been triaged yet. label Mar 4, 2024
@maximlt
Copy link
Member

maximlt commented Mar 18, 2024

I'm not sure. What should this default to?

class Test(param.Parameterized):
    fps = param.Integer(bounds=(None, -1))

@jbednar
Copy link
Member

jbednar commented Mar 19, 2024

I don't think most people would have a strong expectation about what the default would be in either of those cases, so I'd recommend to users that they specify the default explicitly for clarity.

Still, having defaults be automatic is convenient, especially in a notebook context for quick exploratory mini-apps. My gut preference would be to take the midpoint of the two bounds (as the "most typical" value), or if there's only one specified, take that one. But I believe the current practice is to take the lower bound (alas!), so I guess taking the lower bound (if present) and then falling back to the upper bound (if present) is the best we could do.

All that's to say is that I don't object to this proposal, but I also wouldn't mind if other people want to mark it wontfix.

@ahuang11
Copy link
Contributor Author

If it's marked as wontfix I think it should not default to 0 outside the bounds of fps.

@maximlt
Copy link
Member

maximlt commented Mar 19, 2024

If I could choose I'd prefer Param not to set any default values at all, for all Parameters, so I'm not going to advocate for automatically computing smart default values 🙃

But if you want to go down that route, you'll have to define how this new logic behaves in a class inheritance scenario:

import param

class A(param.Parameterized):
    fps = param.Integer(bounds=(1, None))

class B(A):
    fps = param.Integer(bounds=(10, 20))

a = A()
b = B()
print(a.fps, b.fps)  # ?

@ahuang11
Copy link
Contributor Author

ahuang11 commented Mar 19, 2024

My first instinct is 1 and 10, but I don't mind it being wontfix, as long as the default is None (right now it's 0 if I recall).

@philippjfr
Copy link
Member

My first instinct is 1 and 10, but I don't mind it being wontfix, as long as the default is None (right now it's 0 if I recall).

In principle I'm in full agreement but in practice this is a extremely difficult change to make because many users may implicitly be depending on the default being there.

@jbednar
Copy link
Member

jbednar commented Mar 19, 2024

Maybe the most conservative approach is to raise an explicit error when the computed default is invalid for the range, telling the user they need to set an appropriate default.

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

No branches or pull requests

4 participants