-
-
Notifications
You must be signed in to change notification settings - Fork 81
Add a default_factory attribute to Parameter #1092
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
Conversation
e7a5a0b to
f0c7f12
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1092 +/- ##
==========================================
+ Coverage 89.15% 89.21% +0.05%
==========================================
Files 9 9
Lines 4685 4709 +24
==========================================
+ Hits 4177 4201 +24
Misses 508 508 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
philippjfr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks good. I left comments mostly with suggestions on wording, which you can accept or not, though be cautious because editing notebooks in GH is always error prone.
Co-authored-by: Philipp Rudiger <prudiger@anaconda.com>
Co-authored-by: Philipp Rudiger <prudiger@anaconda.com>
Co-authored-by: Philipp Rudiger <prudiger@anaconda.com>
Co-authored-by: Philipp Rudiger <prudiger@anaconda.com>
|
Thanks for your suggestions, I felt the wording was pretty mediocre but somehow refused to ask an LLM to help me, time to give up 🙃 It's much better now! |
|
@philippjfr question on naming. I went for I know that in the past I've sometimes been a bit confused with the On my end, I don't have any strong opinion on naming this, and I'm fine with |
|
My vote is to stick with |
|
I'm fine with |
jbednar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good to me. Thanks for taking care of this! I hope my proposed wording works, but if not, please adapt it to be more accurate but do try to be more explicit in the way I'm suggesting. I see that it can be tricky to describe in the case of on_class=True!
doc/user_guide/Parameters.ipynb
Outdated
| "Each Parameter type can define additional behavior and associated metadata, but the metadata supported for all Parameter types includes:\n", | ||
| "\n", | ||
| "- **default**: Default value for this parameter at the class level, which will also be the value at the Parameterized instance level if it hasn't been set separately on the instance.\n", | ||
| "- **default_factory**: Optional callable to generate the attribute value. The callable can either be passed directly (in which case it is called with 0 arguments), or can be wrapped in a `DefaultFactory` for advanced use cases (in which case the factory is called with the arguments `cls`, `self` and `parameter`). `default_factory` takes precedence over `default` when set. On instance creation, the factory is called once the Parameterized instance is initialized.\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the attribute value" seems confusing here, as it doesn't have any apparent linkage to the default metadata item defined just previously. Maybe:
| "- **default_factory**: Optional callable to generate the attribute value. The callable can either be passed directly (in which case it is called with 0 arguments), or can be wrapped in a `DefaultFactory` for advanced use cases (in which case the factory is called with the arguments `cls`, `self` and `parameter`). `default_factory` takes precedence over `default` when set. On instance creation, the factory is called once the Parameterized instance is initialized.\n", | |
| "- **default_factory**: Optional callable to generate the `default` value for this parameter when an instance of this class is created. The callable can either be passed directly (in which case it is called with 0 arguments), or can be wrapped in a `DefaultFactory` for advanced use cases (in which case the factory is called with the arguments `cls`, `self` and `parameter`). `default_factory` takes precedence over `default` when set. On instance creation, the factory is called once the Parameterized instance is initialized.\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not quite correct as the factory is not called when the Parameter instance is created, but when the Parameterized instance is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 96fa2cd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops; that's what I meant!
doc/user_guide/Parameters.ipynb
Outdated
| "The `default_factory` can be any callable that can be invoked without requiring positional arguments (e.g. `def foo(): ...`, `def foo(*args): ...`, `def foo(**kwargs): ...`, etc.). In addition, the callable can be wrapped as an instance of `param.parameterized.DefaultFactory`, in which case it is called with the three arguments `cls`, `self`, and `parameter`:\n", | ||
| "\n", | ||
| "- When an instance is created, the callable receives the `Parameterized` class as `cls`, the instance itself as `self`, and the instance-level `Parameter` object as `parameter`.\n", | ||
| "- During class creation (typically when a module defining `Parameterized` classes is imported), and only if the `DefaultFactory` is initialized with `on_class=True` (default is `False`), the callable is instead passed the `Parameterized` class as `cls`, `None` for `self` (since no instance exists yet), and the class-level `Parameter` object as `parameter`. This is the only case where the callable can influence the *class-level* attribute value.\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explicitly state what "This" is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9a31616
Co-authored-by: James A. Bednar <jbednar@continuum.io>
Resolves #508
In the simple and likely most common case,
default_factoryis set with a callable that is called internally on instance creation without any argument. This is useful for example to set a UUID on each new instance, or a created_at field:My original motivation for
default_factorywas to find a way to re-implement the behavior of thenameparameter all Parameterized classes have. To that end, the factory callable can be wrapped in aDefaultFactoryinstance, which will let Param know that it needs to call it with the three argumentscls,self, andparameter. InstantiatingDefaultFactorywithon_class=Trueenables the factory to also be called on class creation (in which caseselfis passed None).On instance creation, the factory is called after the instance has been internally marked as initialized. I made that choice as:
parameterargument, or viaself.param[<pname>]) is an instance-level parameter, and not class-level (only class-level parameters can be obtained before the class is initialized), which is less confusing and error-prone, I thinkReviewing
testdefaultfactory.pyis recommended to understand how this all works.