Add simplified param.depends and watch mechanisms #253
Conversation
philippjfr
force-pushed
the
param_mthd_deps_simplified
branch
from
cddf44b
to
f73d6d8
Compare
philippjfr
force-pushed
the
param_mthd_deps_simplified
branch
from
f73d6d8
to
024edb5
Compare
|
|
||
|
|
||
| # (thought I was going to have a few decorators following this pattern) | ||
| def accept_arguments(f): |
There was a problem hiding this comment.
Would be helpful to have docstrings on some of these new functions
param/parameterized.py
Outdated
|
|
||
| # TODO: this is a python 3.6+ thing; we can presumably do this in | ||
| # Parameterized.__new__ for older pythons. (And merge with objtype | ||
| # slot.) |
There was a problem hiding this comment.
@ceball, can you work out what to do here for when not in 3.6?
param/parameterized.py
Outdated
| # (not specific to this change, but regarding attrib_name, owner: | ||
| # what if someone re-uses a parameter object across different | ||
| # classes? maybe we should raise an error if attrib name,owner | ||
| # already set) |
There was a problem hiding this comment.
Seems like that would be polite of us but is not required for merging.
| @@ -472,24 +564,38 @@ def __set__(self,obj,val): | |||
| object stored in a constant or read-only Parameter (e.g. the | |||
| left bound of a BoundingBox). | |||
| """ | |||
| # TODO: simplify this method! | |||
| @@ -979,6 +1085,65 @@ def inspect_value(self_,name): # pylint: disable-msg=E0213 | |||
|
|
|||
| return value | |||
|
|
|||
|
|
|||
| def params_depended_on(self_,name): | |||
There was a problem hiding this comment.
This and several more below need docstrings...
There was a problem hiding this comment.
I'm also not sure about the names yet but we can iterate on that outside this PR.
There was a problem hiding this comment.
I agree - will be part of an issue covering things that need to be sorted out before the next release.
param/parameterized.py
Outdated
| # add watched dependencies | ||
| # | ||
| # TODO: This isn't great. Also note anything here will happen | ||
| # for every instantiation. |
There was a problem hiding this comment.
This bit seems to be pretty expensive, it seems to take about 4x longer than before to instantiate a parameterized class.
There was a problem hiding this comment.
Did it get any better? I'd expect it to be a bit better than before (and minimal impact when there's no watching), but there are still improvements I plan to make (future PR).
(Includes restoring more explicit args for depends().)
…ibutes at instantiation time).
Dependencies: support more pythons and begin storing dependency info in more convenient way
|
With #255, does this cover what yuo need for now? When merged, I'll make an issues about things that must be cleaned up before release, and things that could be improved/added in the future. |
Yes I think so. Looking forward to seeing this merged. |
|
Who's handling that - me or you? I don't mind either way, just let me know if I should do it... |
Missed your question about subscribe on the other PR. Happy to merge both once that's deleted. |
Also simplified signature of watch (and unwatch).
|
Looking at the diff I do have a number of suggestions to make. To avoid slowing things down, I won't write anything down just yet so that we can merge sooner rather than late. |
I have some suggestions too. But yes, I think until we've settled on the functionality we want, we should hold off. Unless you're talking about something that's an error... |
|
I just double checked and after updating |
Supersedes #250 by making a number of simplifications:
viewableargument forparam.dependscacheargument forparam.depends(can be added back later)eagerkeywordwatchinparam.depends