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

Initial go at methods depending on parameters #250

Merged
merged 1 commit into from
Aug 28, 2018
Merged

Conversation

ceball
Copy link
Contributor

@ceball ceball commented Aug 16, 2018

@jbednar
Copy link
Member

jbednar commented Aug 18, 2018

Looks fabulous! In general, I completely agree with your TODO items and wherever you say something should be done; yes, it should. Minor issues:

  • We verbally agreed to rename "eager" to "watch", and after reading over it I still agree.
  • The examples in the notebook make good documentation, but the notebook should be changed so that it will work with Run All. Right now, the cycle example or some other error near the end makes the ones at the beginning fail to work any more. You could e.g. comment out the call that instantiates the error-containing class?
  • The syntax summary should say that "subobj.param:constant" is supported, assuming it is.
  • Yes, we can allow metadata to vary per instance, but I'd prefer that to be a separate PR and one with very clear motivating examples. It shouldn't be hard to find such examples (e.g. for range), but the examples should make various associated issues clearer.
  • For your "notes":
    • "I think viewable should be a separate decorator, and it should be for declaring things about the return type." -- I agree.
    • "I think it should be possible to declare also that a method modifies parameters." -- I agree.
    • "things should be in param namespace" -- I agree.
    • "Changes to mutable objects not detected" -- Seems ok for now for parameter dependencies, but not for memoize/cache -- those really do need to know when things change.
    • "You're don't hear about y changing if the new object x has a different value for y" -- If you care about that case, can you not just also watch the parameter whose value is x?
    • cycles -- Detecting them is nice but doesn't seem required.

@jlstevens
Copy link
Contributor

I just had a thought about the concept of the viewable decorator: maybe it is a subclass of a more general metadata decorator that allows param to track any metadata that a user might want to associate with particular methods?

@philippjfr
Copy link
Member

philippjfr commented Aug 21, 2018

My strong preference would be to leave any viewable or output type declaration for a separate PR. I think it was important for @ceball to replicate existing demos but it's not yet clear to me how it should work and/or if it should even be handled automatically.

It has some overlap with my layout work in pyviz_panel and I'd like to work out how those will interact. Once that is properly supported I would actively discourage users from using parambokeh.Widgets to combine widgets and viewables and would instead recommend users compose the widgets and plots explicitly using the Row/Column layouts. In the long term I'm of course not averse to providing easy ways to declare some combination of widgets and plots by declaring the output types in some way but I don't think it should be part of a first pass and it seems orthogonal to this PR.

@jlstevens
Copy link
Contributor

jlstevens commented Aug 21, 2018

My strong preference would be to leave any viewable or output type declaration for a separate PR.

I agree.

Edit: Oops! I was about to make a comment then realized I needed to think about it more...then accidentally clicked 'Close pull request' in case you are working about what happened below..

@jlstevens jlstevens closed this Aug 21, 2018
@jlstevens jlstevens reopened this Aug 21, 2018
@philippjfr
Copy link
Member

I'm going to start building functionality in the new pyviz_panels package using this PR, e.g. to allow dynamically updating a subplot. It would therefore be great if we could merge this in the near term. Once the pyviz_panels work is complete we can rewrite the layout portions in parambokeh by building on that work. I think that work will also provide a path forward for displaying param sub-objects.

@jbednar
Copy link
Member

jbednar commented Aug 22, 2018

@philippjfr, @ceball can't work on this today or tomorrow. To make progress, do you think you could remove viewable=True and instead put something in parambokeh that can do the same thing but with a provided method name? Then we can merge this and move on.

@philippjfr
Copy link
Member

To make progress, do you think you could remove viewable=True and instead put something in parambokeh that can do the same thing but with a provided method name?

I don't want or need viewable=True at all for the time being.

@jbednar
Copy link
Member

jbednar commented Aug 22, 2018

Sure, but pulling it out will break the examples here. I guess you could replace the examples with something equivalent that doesn't need viewable?

@philippjfr
Copy link
Member

I guess you could replace the examples with something equivalent that doesn't need viewable?

Sure I can do that. The thing is, I'm not even sure how to replicate those examples locally since I don't know what modifications were made to parambokeh and paramnb to make them work.

@philippjfr
Copy link
Member

Making good progress on pyviz_panels but one thing I'm missing to make everything work cleanly is a way to stop watching something.

@jbednar
Copy link
Member

jbednar commented Aug 23, 2018

Can't take your eyes off it, eh? :-) I'm happy for there to be an unwatch call...


# TODO: this is a python 3.6+ thing; we can presumably do this in
# Parameterized.__new__ for older pythons. (And merge with objtype
# slot.)
Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate? Does this mean it currently only works with Python 3.6?

if old is not NotImplemented:
for subscriber in self.subscribers[name]:
subscriber(Change(what=name,attribute=self._attrib_name,obj=None,cls=self._owner,old=old,new=value))

Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't an 'attribute', it is a 'parameter' or a parameter name...

Copy link
Member

Choose a reason for hiding this comment

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

Confused, a parameter that isn't actually a Parameter is an attribute.

Copy link
Member

Choose a reason for hiding this comment

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

Right; this is part of setattr, which handles Parameter values and regular attributes both.

Copy link
Contributor

@jlstevens jlstevens Sep 10, 2018

Choose a reason for hiding this comment

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

We can't detect changes to regular attributes though. I am pretty sure Change can only propagate to callbacks for parameters, not attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jlstevens is right, in that Change can only be about parameters of a parameterized (instance or class), not about regular attributes of a parameterized.

Just in case the limited view on github+my coding is confusing people...this hacky __setattr__ method is to deal with changes to attributes of a Parameter object (maybe we call those things parameter metadata). So, not about parameters and attributes of a parameterized object. The naming is really not great. what refers to e.g. value, bounds, constant, etc. attribute refers to the name which the owning Parameterized has for the Parameter. And obj and cls are necessary on a Change in general because it could come from a parameterized instance or class.

(My opinion...I would still prefer not to review the code changes yet. The Change object is probably not exactly what I'd like, and the setattr method is definitely not what I'd like, but I'd prefer to settle the functionality/behavior we want before investing time in the underpinnings. E.g. we may end up not caring about parameter metadata...or at the other extreme, we may end up wanting it to be per instance (whereas it's only per class). Having said all that, I should clean up all the issues and PRs relating to this stuff, so we have one place to discuss, because you may already have started to decide about some of these things, given you're using them...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I just realized why you might be looking at this stuff - I didn't see #259 until now. You're brave! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe keep all code comments to #253, since that what got merged, and is where most such comments are. (Some stuff from this PR is not/will not be merged.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ceball Thanks for the explanation. The change to __setattr__ on Parameterss makes sense given those are attribute changes you can look out for (i.e what). It sounds like you agree that attribute really has to be a parameter (or to be precise, a parameter name).

I share your feelings about wanting to hold back so we can refine and polish things better. That said, I do think the parallel setting (#259) will be important and therefore something to consider now.

I think I found a way to implement that in a way that is largely orthogonal to everything else which means I have opted to hold back on making any other code comments right now (which I agree belong in #253).

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