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

Proposal for parameter attribute inheritance changes in param v2 #113

Closed
ceball opened this issue Apr 5, 2015 · 2 comments · Fixed by #605
Closed

Proposal for parameter attribute inheritance changes in param v2 #113

ceball opened this issue Apr 5, 2015 · 2 comments · Fixed by #605
Labels
API breaking Affects the api in a non bug fixy way/consider version or name change component: type/value stuff system of parameter type/value checking, inheritnace, etc etc type-feature Feature request
Milestone

Comments

@ceball
Copy link
Member

ceball commented Apr 5, 2015

(draft proposal!)

Currently, Parameter attribute inheritance is a bit inconsistent (or
at least not well documented). By 'Parameter attribute', we mean
e.g. the default attribute (slot) of Parameter, or the bounds
slot of Number. The main problem in param v1 is that None was
originally used to mean 'not set', but eventually we started to use
None as a valid value too.

We're proposing to address these inheritance-related inconsistencies
in param v2. Below we list some proposed changes, and highlight
current inconsistencies using some example classes.

Changes:

  • All parameter attributes will default to NotImplemented ("not
    set"). Currently, some attributes default to None, while many
    default to other things (e.g. 1.0 for Number).
  • If not set, a Parameter attribute will inherit from the attribute of
    a Parameter of the same name defined in the superclass of the owning
    Parameterized. Currently, this inheritance may or may not happen
    (see examples later).
  • If a Parameter attribute fails to get a value when inheriting as
    above, there will be an exception (nothing can be left 'unset').
  • Parameters will no longer have a default value at the Parameter
    class level (it's up to the person using that Parameter to supply
    such a default). Note that this means e.g. the List parameter can no
    longer default to an empty list (at the List class level).
  • Parameter class authors can, however, declare default values for
    attributes other than default (i.e. for the meta parameters). For
    instance, the author of the List parameter class can say
    instantiate=True by default for List parameters; then, when a List
    parameter is declared in a Parameterized class, if the class author
    does not specify a value for instantiate, and the Parameterized
    class's superclass (if any) does not have a List (subclass) of the
    same name with a value for instantiate specified, then
    instantiate will be True for that List parameter.
  • If you define a parameter x in a subclass, that parameter will have
    to be the same type as/a subclass of the x parameter class in owning
    parameterized's superclass. Currently we don't enforce this.

Some classes (slightly modified from current reality for brevity) for
examples...

    class Parameter(object):
        ...
        def __init__(self,
                     default=None,
                     doc=None,
                     allow_None=False)


    class Number(Parameter):
        ...
        def __init__(self,
                     default=0.0,
                     bounds=None,
                     inclusive_bounds=(True,True),
                     **kw)

    class List(Parameter):
        ...
        def __init__(self,
                     default=[],
                     bounds=(0,None),
                     **kw)


    class A(Parameterized):
        a = Number(default=None, 
                   doc="a number",
                   allow_None=True,
                   bounds=(0.5,1.5), 
                   inclusive_bounds=(True,False))

        b = List(default=[1,2,3],
                 bounds=(3,3))


    class B(A):
        a = Number(default=1.2)
        b = List(default=None)


    class C(B):
        a = Number(default=None)
        b = Number(default=10)

For the classes above, considering first the 'a' parameter, I think
the following is currently how things end up after the parameterized
classes have been created:

  A.a
    default=0.0 
    doc="a number"
    allow_None=True
    bounds=(0.5,1.5)
    inclusive_bounds=(True,False)

  B.a
    default=1.2
    doc="a number"
    allow_None=False
    bounds=(0.5,1.5)
    inclusive_bounds=(True,True)

  C.a
    default=0.0
    doc="a number"
    allow_None=False
    bounds=(0.5,1.5)
    inclusive_bounds=(True,True)

Some notes:

  • A.a default should be None
  • B.a allow_None should be True? Maybe: this will make the logic
    clearer with fewer special cases. But maybe not: it's ok for a
    subclass to have more restriction than its superclass, and None
    might not be meaningful for B even though it's meaningful for A.
  • B.a inclusive_bounds should be (True, False).
  • C.a default should be None.

Now considering the 'b' parameter...

  A.b
    default=[1,2,3]
    bounds=(3,3)

  B.b
    default=[1,2,3]
    bounds=(3,3)

  C.b
    default=10
    bounds=(3,3) # I think; bounds will inherit after default is
                   checked?

Notes:

  • Shouldn't be possible to declare C.b as a Number because Number's
    not a subclass of List.
  • (Ignoring the above,) C.b shouldn't be able to have a default
    outside the bounds. (Default values be checked after bounds are
    inherited).
@jbednar
Copy link
Member

jbednar commented Apr 6, 2015

That all looks correct to me, and should be done for the 2.0 release of Param.

@jbednar jbednar added this to the 2.0 milestone Apr 6, 2015
@jlstevens
Copy link
Contributor

Sounds good. There may be places where we assumed the class defaults for the parameters (e.g empty list as the default for List) but then those cases will need to be updated.

I do think that using NotImplemented to denote 'not set' certainly does seems sensible and a lot safer than what we are doing now!

@ceball ceball added the type-feature Feature request label Jun 16, 2017
@ceball ceball added API breaking Affects the api in a non bug fixy way/consider version or name change component: type/value stuff system of parameter type/value checking, inheritnace, etc etc labels Apr 13, 2020
@ceball ceball removed this from the 2.0 milestone Apr 13, 2020
@jbednar jbednar added this to the 2.0 milestone Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API breaking Affects the api in a non bug fixy way/consider version or name change component: type/value stuff system of parameter type/value checking, inheritnace, etc etc type-feature Feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants