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

Reimplement ObservablePairs and eliminate notify keyword #56

Merged
merged 2 commits into from
Jan 4, 2021

Conversation

timholy
Copy link
Member

@timholy timholy commented Jan 1, 2021

The old implementation of ObservablePairs seems unnecessarily
complicated. Perhaps more importantly, this change allows us
to eliminate the notify kwarg of setindex!, which should reduce
type-diversity and may help reduce latency.

EDIT: I also suspect that the current implementation has a bug if you make two pairs, one with a and b and the other with b and c and use the same update function for both. This implementation should not suffer from any such problems.

The old implementation of ObservablePairs seems unnecessarily
complicated. Perhaps more importantly, this change allows us
to eliminate the `notify` kwarg of `setindex!`, which should reduce
type-diversity and may help reduce latency.
@timholy
Copy link
Member Author

timholy commented Jan 4, 2021

I'd also propose we rename f and g to something more memorable that doesn't require one to check the source code to see which one does which. Maybe first2second and second2first?

@timholy timholy requested a review from piever January 4, 2021 10:40
@piever
Copy link
Contributor

piever commented Jan 4, 2021

Thanks! I agree that your implementation of ObservablePair is cleaner, but I'm not 100% sure if the notify keyword argument can be eliminated. Other than the ObservablePair application, I can't think of many other applications, and your done[] trick could probably be applied to those as well.

Pinging @jkrumbiegel because I think some of his Makie code (link) relies on the notify keyword. If that can also be fixed, I guess we can get rid of the keyword.

@timholy
Copy link
Member Author

timholy commented Jan 4, 2021

Thanks @piever!

So folks are aware, you can update the observable with obs.val = x and not notify. My next commit in this series (see #55 (comment); currently I have 8 likely PRs queued after this one 😄, not including the ones already submitted) will split setindex! into

obs.val = x
notify(obs)

to make it easier to control notification manually. (Currently we have notify! which calls setindex! but that seems a bit backwards; setindex! should be built from notify instead.)

Base.setindex!(observe(observable), val; notify=notify)
end

function setexcludinghandlers(observable::AbstractObservable, val, pred=x->true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that this is internal, but I guess we could keep something along the lines of

setexcludinghandlers(obs::AbstractObservable, val) = observe(obs).val = val

so that the "silent update" is not done by touching an internal field as in #56 (comment). In particular, the observe(obs) pattern is needed because Interact widgets are AbstractObservables, whose "concrete observable" can be accessed with observe(obs). As mentioned in that comment, setindex! would then just by setexcludinghandlers followed by notify!.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

@piever
Copy link
Contributor

piever commented Jan 4, 2021

Ok, I've just left a small comment (to basically keep a method for the silent update, because accessing the field directly won't work on AbstractObservables), but other than that this looks good to me.

@timholy timholy merged commit 1c3ace9 into master Jan 4, 2021
@timholy timholy deleted the teh/notify1 branch January 4, 2021 22:21
@timholy
Copy link
Member Author

timholy commented Jan 4, 2021

Thanks @piever!

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.

2 participants