Opened 2 years ago

Closed 2 years ago

#24760 closed enhancement (fixed)

convert cluster interact to jupyter notebook

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.3
Component: notebook Keywords: cluster
Cc: sage-combinat, stumpc5, jason, etn40ff Merged in:
Authors: Frédéric Chapoton Reviewers: Christian Stump
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: a4f01c1 (Commits) Commit: a4f01c13351fac88a2b4def5b448385a404a4724
Dependencies: Stopgaps:

Change History (61)

comment:1 Changed 2 years ago by jdemeyer

What is the "cluster interact"?

comment:2 Changed 2 years ago by chapoton

one second, branch coming

comment:3 Changed 2 years ago by chapoton

  • Branch set to public/24760
  • Commit set to 23fd5192eeb5dac5adf8d2c39b9fc9e663d0802f

and help would be welcome..


New commits:

23fd519first tentative of interact conversion for cluster mutation

comment:4 Changed 2 years ago by chapoton

I failed to make it work. This is related to #24595

Last edited 2 years ago by chapoton (previous) (diff)

comment:5 Changed 2 years ago by jdemeyer

Just a few comments:

  1. Ideally, you would return the interact instead of displaying it. This means replacing @interact by @interact.widget and then returning player.
  1. I wouldn't bother checking for Jupyter. In the worst case, you get a warning like
    [SageTerminalApp] WARNING | Widget Javascript not detected.  It may not be installed or enabled properly.
    Widget Javascript not detected.  It may not be installed or enabled properly.
    

comment:6 Changed 2 years ago by git

  • Commit changed from 23fd5192eeb5dac5adf8d2c39b9fc9e663d0802f to a747dbaef648afcbb404c618fc98308c49132a12

Branch pushed to git repo; I updated commit sha1. New commits:

a747dbareturning a widget

comment:7 follow-up: Changed 2 years ago by chapoton

Thanks for the hints. I have changed the code accordingly.

But alas, this is still not doing what it should. Namely, I would like to use a global variable, but of course this does not make much sense..

The problem is that it must distinguish between clicks on the numbers at top (first parameter k) that means 'doing a mutation at vertex k" (changing the displayed graph) to clicks on the other checkboxes, that just mean display or not a few additional fixes, but do not perform any mutation.

Last edited 2 years ago by chapoton (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 2 years ago by jdemeyer

Pro tip: if you are thinking about using ipywidgets interacts non-trivially, you shouldn't be using interacts.

There is a difference in philosophy between sagenb interact and ipywidgets interacts: with sagenb, the only way to do anything with widgets is to use @interact. This leads to ugly solutions such as the old code for the cluster interact (did you see how it abuses the lists sft, sss, ssv ssm, ssl?)

With ipywidgets on the other hand, the widgets are the basic building blocks. There is a lot of stuff you can do with widgets without using @interact. They do provide an interact function for convenience, but it's really just a wrapper over the underlying widgets. You might as well wrap the widgets yourself.

In more detail: when you add @interact to a function, it gets an attribute .widget which is an instance of ipywidgets.widgets.interaction.interactive:

sage: from ipywidgets import interact
sage: @interact
....: def f(x=0, y=True, z=range(4)): pass
[SageTerminalApp] WARNING | Widget Javascript not detected.  It may not be installed or enabled properly.
Widget Javascript not detected.  It may not be installed or enabled properly.
sage: widget = f.widget
sage: type(widget)
<class 'ipywidgets.widgets.interaction.interactive'>

This widget is really just a special vertical box of widgets:

sage: type(f.widget).__bases__
(<class 'ipywidgets.widgets.widget_box.VBox'>,)
sage: f.widget.children
(IntSlider(value=0, min=0, max=1, step=1, description=u'x'),
 Checkbox(value=True, description=u'y'),
 Dropdown(value=0, options=[0, 1, 2, 3], description=u'z'),
 Output())

If you would manually construct a VBox of those 4 widgets, you would already get the interact but without the callbacks to make it interactive and to display the output.

comment:9 Changed 2 years ago by chapoton

Hmm. I understand, sort of. But I am going to have a hard time if I ever try to do something like that. I guess it will require widgets-communication somewhere.

comment:10 Changed 2 years ago by git

  • Commit changed from a747dbaef648afcbb404c618fc98308c49132a12 to bf78c5523f4d628fbdc9b1867894e849b5cc65d2

Branch pushed to git repo; I updated commit sha1. New commits:

bf78c55trying to use widgets

comment:11 Changed 2 years ago by chapoton

ok, I made a tentative use of widgets. But this is not quite working.

comment:12 Changed 2 years ago by jdemeyer

I meant not using interact at all.

comment:13 Changed 2 years ago by git

  • Commit changed from bf78c5523f4d628fbdc9b1867894e849b5cc65d2 to 2a094ddffffe48374cbe33512c080ee77c0b1b21

Branch pushed to git repo; I updated commit sha1. New commits:

2a094ddtrying harder to use widgets

comment:14 Changed 2 years ago by chapoton

Here is my latest try..

comment:15 Changed 2 years ago by git

  • Commit changed from 2a094ddffffe48374cbe33512c080ee77c0b1b21 to e15fa9024a535092957013e1a1ea961b14a8623a

Branch pushed to git repo; I updated commit sha1. New commits:

e15fa90another tentative of using widgets

comment:16 Changed 2 years ago by chapoton

Damn. This sort of works, but the output accumulates instead of being replaced..

comment:17 Changed 2 years ago by git

  • Commit changed from e15fa9024a535092957013e1a1ea961b14a8623a to 5155ee43357b00759cd3ef800ed86eabddc7bccc

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5155ee4interact conversion to ipywidgets for cluster mutation

comment:18 Changed 2 years ago by chapoton

now works quite well, except that one cannot perform the same action twice (related to #24595)

comment:19 Changed 2 years ago by git

  • Commit changed from 5155ee43357b00759cd3ef800ed86eabddc7bccc to 00072e5bc330fb4ac9dc6b6d89961fbde514b95a

Branch pushed to git repo; I updated commit sha1. New commits:

cb6abaaMerge branch 'public/24760' in 8.2.b6
00072e5fixing the widgets

comment:20 Changed 2 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Cc sage-combinat stumpc5 added
  • Status changed from new to needs_review

now looks good. Except that it is very easy to have repeated clicks without wanting them.

comment:21 Changed 2 years ago by stumpc5

Great, thanks for making this work again! -- everything seems to work as expected.

Concerning the usage, I have two issues that I believe had worked better before:

  1. currently nothing is shown when initially running S.interact(). The quiver and the additional info only show after the first mutation.
  1. the "window" showing the buttons and the info seems to be of a fixed height, so I have to do a mutation and then scroll down inside this window as shown in https://screenshots.firefox.com/nTDuxI5z46neAILs/localhost

I cannot review the code whatsoever, but the usage is fine (and the two issues are only worth solving if they are easily fixed).

comment:22 Changed 2 years ago by git

  • Commit changed from 00072e5bc330fb4ac9dc6b6d89961fbde514b95a to ba5044eaf54e3f701f0b30cf308bf8c8d9931f17

Branch pushed to git repo; I updated commit sha1. New commits:

ba5044eone detail

comment:23 Changed 2 years ago by chapoton

This is not yet working as expected...

comment:24 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Regarding

        # not clear how to get the right behaviour here
        # doing both has some side effects (one click : two mutations)
        mut_buttons.on_msg(do_mutation)
        mut_buttons.observe(do_mutation, 'value')

is it not sufficient to keep only the on_msg() call?

comment:25 Changed 2 years ago by chapoton

With on_msg only, the button 0 seems to be always on (darker), and triggers a mutation at the previous vertex which is not 0. Like if the button 0 does not change the value, but triggers a msg.. I see really strange effects..

comment:26 Changed 2 years ago by git

  • Commit changed from ba5044eaf54e3f701f0b30cf308bf8c8d9931f17 to 550f13d7dc4881db12d75df26ea06f2d42bbf8e2

Branch pushed to git repo; I updated commit sha1. New commits:

550f13dfix doctest

comment:27 Changed 2 years ago by git

  • Commit changed from 550f13d7dc4881db12d75df26ea06f2d42bbf8e2 to ba69a8cd3f891dfed93004c79cb966182abe24f2

Branch pushed to git repo; I updated commit sha1. New commits:

ba69a8ctry to add image at start

comment:28 Changed 2 years ago by chapoton

Is the ToggleButtons widget broken or what ? When trying

n=5
widgets.ToggleButtons(options=list(range(n)),
                                            button_style='',
                                            description='Mutate at: ')

in my notebook (using chrome) the selection always remains on 0.

EDIT: same wrong behaviour with python2 kernel.

Last edited 2 years ago by chapoton (previous) (diff)

comment:29 Changed 2 years ago by git

  • Commit changed from ba69a8cd3f891dfed93004c79cb966182abe24f2 to ab4f0869aac226669c8b66b435575e9845ff221a

Branch pushed to git repo; I updated commit sha1. New commits:

ab4f086just using on_msg

comment:30 Changed 2 years ago by chapoton

Last edited 2 years ago by chapoton (previous) (diff)

comment:31 Changed 2 years ago by chapoton

  • Keywords cluster added

comment:32 Changed 2 years ago by chapoton

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:33 Changed 2 years ago by git

  • Commit changed from ab4f0869aac226669c8b66b435575e9845ff221a to 5fd5f2bdd5bcd123d7623e8ed0d8f329f99d5d94

Branch pushed to git repo; I updated commit sha1. New commits:

5fd5f2bbetter buttons

comment:34 Changed 2 years ago by chapoton

  • Cc jason added

Jason, may you please help with the ipywidgets upstream issue ?

comment:35 Changed 2 years ago by chapoton

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

comment:36 Changed 2 years ago by chapoton

This will have to wait for ipywidgets 7.1.3, probably.

comment:37 Changed 2 years ago by chapoton

  • Status changed from needs_work to needs_review

Now this is fully ready, but will not work perfectly until ipywidgets is updated to a version including the upstream changes.

Should we wait, give that the actual interact is currently fully broken ?

comment:38 Changed 2 years ago by chapoton

  • Status changed from needs_review to needs_work

doctest does not pass.. sigh...

comment:39 Changed 2 years ago by stumpc5

fyi: when clicking the "0" button, it still gets mutated at vertex "1", this might be part of the bug in the widget though.

comment:40 Changed 2 years ago by stumpc5

For future work reference: this seems to work only for ClusterQuiver but not for ClusterSeed (I get an error that it works only in the notebook when doing

sage: Q = ClusterQuiver(['A',3])
sage: Q.interact()

and I also get an error if the vertex labels are "a","b","c":

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
/home/stumpc5/Programs/sage/local/lib/python2.7/site-packages/sage/combinat/cluster_algebra_quiver/cluster_seed.pyc in refresh(w)
   1111                 clear_output(wait=True)
   1112                 if show_lastmutation.value:
-> 1113                     self.show(fig_size=fig_size, circular=circular, mark=k)
   1114                 else:
   1115                     self.show(fig_size=fig_size, circular=circular)

/home/stumpc5/Programs/sage/local/lib/python2.7/site-packages/sage/combinat/cluster_algebra_quiver/cluster_seed.pyc in show(self, fig_size, circular, mark, save_pos, force_c, with_greens, add_labels)
   1043             quiver = self.quiver()
   1044 
-> 1045         quiver.show(fig_size=fig_size, circular=circular,mark=mark,save_pos=save_pos, greens=greens)
   1046 
   1047     def interact(self, fig_size=1, circular=True):

/home/stumpc5/Programs/sage/local/lib/python2.7/site-packages/sage/combinat/cluster_algebra_quiver/quiver.pyc in show(self, fig_size, circular, directed, mark, save_pos, greens)
    671         """
    672         n, m = self._n, self._m
--> 673         plot = self.plot( circular=circular, directed=directed, mark=mark, save_pos=save_pos, greens=greens)
    674         if circular:
    675             plot.show( figsize=[fig_size*3*(n+m)/4+1,fig_size*3*(n+m)/4+1] )

/home/stumpc5/Programs/sage/local/lib/python2.7/site-packages/sage/combinat/cluster_algebra_quiver/quiver.pyc in plot(self, circular, center, directed, mark, save_pos, greens)
    611                 partition = (nlist, mlist, [mark])
    612             else:
--> 613                 raise ValueError("The given mark is not a vertex of self.")
    614         else:
    615 

ValueError: The given mark is not a vertex of self.

comment:41 Changed 2 years ago by git

  • Commit changed from 5fd5f2bdd5bcd123d7623e8ed0d8f329f99d5d94 to a4cc657d1f4db51bf82543b193acbbc8199c9b1f

Branch pushed to git repo; I updated commit sha1. New commits:

1a2fea9Merge branch 'public/24760' in 8.2.b8
e1fc815fixing detail and doctest
a4cc657new interact also for cluster quivers

comment:42 follow-up: Changed 2 years ago by chapoton

I have done a copy-paste for the ClusterQuiver interact, but removing the variables.

Clicking on 0 will work when ipywidgets is updated.

For labels, maybe this can wait. I propose to keep more enhancements for a later ticket. The really important task done here is to get rid of the import of sagenb, to move towards python3.

EDIT: bot is morally green

Last edited 2 years ago by chapoton (previous) (diff)

comment:43 Changed 2 years ago by chapoton

  • Status changed from needs_work to needs_review

comment:44 in reply to: ↑ 42 ; follow-up: Changed 2 years ago by jdemeyer

Replying to chapoton:

I have done a copy-paste for the ClusterQuiver interact, but removing the variables.

If you need to copy/paste code, you're doing it wrong.

Why not factor this out into a subclass or separate function?

Clicking on 0 will work when ipywidgets is updated.

Could we not backport the upstream patch?

comment:45 Changed 2 years ago by jdemeyer

  • Component changed from combinatorics to notebook
  • Description modified (diff)
  • Status changed from needs_review to needs_work

comment:46 in reply to: ↑ 44 Changed 2 years ago by jdemeyer

Replying to jdemeyer:

Could we not backport the upstream patch?

Answering my own question... It's using typescript, which needs to be compiled at packaging-time (I guess), so that is not so trivial.

comment:47 follow-up: Changed 2 years ago by jason

I'm going through ipywidgets issues today in preparation for our next release. There are a few issues I want to address, and hope to have a new release (or at least an RC) out today or tomorrow.

comment:48 Changed 2 years ago by jason

(Next time, feel free to ping me on an ipywidgets issue, as I see those more often than trac notifications)

comment:49 in reply to: ↑ 47 Changed 2 years ago by jdemeyer

Replying to jason:

There are a few issues I want to address, and hope to have a new release (or at least an RC) out today or tomorrow.

In this case, I would rather wait with this ticket until the new release.

comment:50 Changed 2 years ago by jason

I've created a milestone for the release: https://github.com/jupyter-widgets/ipywidgets/milestone/25

Given the number of issues to review from the work today, it may be a few more days before a release, but hopefully by the end of the week.

comment:51 Changed 2 years ago by git

  • Commit changed from a4cc657d1f4db51bf82543b193acbbc8199c9b1f to 05ec0d9404677bd553fbb44281de3536a4995f99

Branch pushed to git repo; I updated commit sha1. New commits:

05ec0d9separating the interact function

comment:52 Changed 2 years ago by stumpc5

I just recompiled your newest version and get

AttributeError: 'ClusterQuiver' object has no attribute 'cluster_variable'

so the ClusterQuiver interact still hopes for variables to exist...

comment:53 Changed 2 years ago by git

  • Commit changed from 05ec0d9404677bd553fbb44281de3536a4995f99 to a4f01c13351fac88a2b4def5b448385a404a4724

Branch pushed to git repo; I updated commit sha1. New commits:

a4f01c1fixing the case of quivers

comment:54 Changed 2 years ago by stumpc5

thanks for taking care of this!

comment:55 Changed 2 years ago by chapoton

  • Report Upstream changed from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.

comment:56 Changed 2 years ago by chapoton

  • Dependencies set to #25074
  • Status changed from needs_work to needs_review

I opened #25074 for the necessary upgrade of ipywidgets.

comment:57 Changed 2 years ago by chapoton

ipywidgets has been updated in the latest beta 8.3.b1

Can someone please re-check that this is ready to go ?

comment:58 Changed 2 years ago by chapoton

  • Cc etn40ff added
  • Dependencies #25074 deleted
  • Milestone changed from sage-8.2 to sage-8.3

This now works fine for me, both for seeds and quivers. Please confirm.

comment:59 Changed 2 years ago by stumpc5

I will only be able to test on Tuesday next week, but if it works for you feel free to give it set it to positive!

comment:60 Changed 2 years ago by chapoton

  • Reviewers set to Christian Stump
  • Status changed from needs_review to positive_review

ok. I am setting to positive now.

comment:61 Changed 2 years ago by vbraun

  • Branch changed from public/24760 to a4f01c13351fac88a2b4def5b448385a404a4724
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.