holoviz-topics / examples

Visualization-focused examples of using HoloViz for specific topics
https://examples.holoviz.org
Creative Commons Attribution 4.0 International
82 stars 24 forks source link

attractors: Modernize_attractors #381

Closed Azaya89 closed 4 months ago

Azaya89 commented 5 months ago

Modernizing an example checklist

Preliminary checks

Change ‘anaconda-project.yml’ to use the latest workable version of packages

Plot API updates (discussed on a per-example basis)

Interactivity API updates (discussed on a per-example basis)

Panel App updates (discussed on a per-example basis)

General code quality updates

Text content

Visual appearance - Example

Visual appearance - Gallery

Workflow (after you have made the changes above)

github-actions[bot] commented 5 months ago

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/. You can also download an archive of the site from the workflow summary page which comes in handy when your dev site built was overriden by another PR (we have a single dev site!).

github-actions[bot] commented 5 months ago

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/. You can also download an archive of the site from the workflow summary page which comes in handy when your dev site built was overriden by another PR (we have a single dev site!).

maximlt commented 5 months ago

I'm currently reviewing this PR but wanted to write a message dedicated to the issue we observed in attractors_panel.ipynb and the widgets pane containing some weird things.

How it should look: image

vs. how it looks at the moment in this PR:

image

We can see that:


Let's look at the original code (before this PR):

diff --git a/before.py b/pr.py
index ee25674..85bc4fe 100644
--- a/before.py
+++ b/pr.py
@@ -2,14 +2,13 @@ import param, panel as pn
 from panel.pane import LaTeX
 pn.extension('katex')

-class Attractors(param.Parameterized):
-    attractor_type = param.ObjectSelector(params.attractors["Clifford"],
-                                          params.attractors, precedence=0.9)
+class Attractors(pn.viewable.Viewer):
+    attractor_type = param.Selector(objects=params.attractors, default=params.attractors["Clifford"], precedence=0.9)

-    parameters = param.ObjectSelector(params, precedence=-0.5, readonly=True)
+    parameters = param.Selector(objects=params.attractors, precedence=-0.5, readonly=True)

-    plot_type = param.ObjectSelector(
-        "points", precedence=0.8, objects=['points', 'line'],
+    plot_type = param.Selector(
+        precedence=0.8, objects=['points', 'line'],
         doc="Type of aggregation to use"
     )

@@ -23,7 +22,7 @@ class Attractors(param.Parameterized):
             self.param.set_param(attractor_type=a)

     @param.depends("attractor_type.param", "plot_type", "n")
-    def view(self):
+    def __panel__(self):
         return datashade(self.attractor_type(n=self.n), self.plot_type,
                          palette[self.attractor_type.colormap][::-1])

The change made to the parameters definition looks suspicious, going from parameters = param.ObjectSelector(params, ...) to parameters = param.Selector(objects=params.attractors, ...). objects should be set to params, not params.attractors. So I made that change locally and now get this error:

TypeError: 'ParameterSets' object is not iterable

params is a ParameterSets instance (a class defined in attractors.py), it's not of the type list or dict that the SelectorParameter expects (getting the same error with ObjectSelector by the way). I'm not sure why the original code used an ObjectSelector. In the old code, the app looks identical if I replace it with params = param.Parameter(objects, ...). However, if I apply the same change in the PR, I see two problems:

  1. The section name is not Attractors but ParameterSets00207.
  2. The buttons in the expanded section have varying widths

image

On these issues:

  1. Param issue https://github.com/holoviz/param/issues/937 for which I have a potential fix.
  2. Panel issue I guess 😃 , perhaps related to the weird alignment issue already noted. I haven't looked into that yet.

And this is what I get when applying the potential fix in Param.

image


EDIT after quickly looking at the Panel issue. I think it has to do with changes in the default margin of the Param pane and changes in the default width of buttons. Running this before creating the Param pane can help making the app looking closer to how it used to be:

pn.Param.margin = 0
# pn.widgets.Button.width = 300  # why doesn't this work?
pn.widgets.Button.param.width.default = 300

image

github-actions[bot] commented 5 months ago

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/. You can also download an archive of the site from the workflow summary page which comes in handy when your dev site built was overriden by another PR (we have a single dev site!).

github-actions[bot] commented 5 months ago

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/. You can also download an archive of the site from the workflow summary page which comes in handy when your dev site built was overriden by another PR (we have a single dev site!).

Azaya89 commented 5 months ago

Looks good, thanks! There seem to be some changes referenced but not yet in the PR, so there may be a version still on @Azaya89 's machine that needs to be pushed here.

I have pushed the updated changes now. However, I'm still not done with the last bit concerning the widgets panel in the attractors_panel.ipynb notebook.

maximlt commented 4 months ago

@Azaya89 this rebase went wrong https://github.com/holoviz-topics/examples/pull/381/commits/561469837ef14cb299555f4565334d64875de90b.

I suggest you revert it, and just copy/paste the changes from #200, it's sometimes the easier way.

Azaya89 commented 4 months ago

@Azaya89 this rebase went wrong 5614698.

I suggest you revert it, and just copy/paste the changes from #200, it's sometimes the easier way.

Yeah, it was just the merge conflicts in the dodo.py and build_one.yml files I didn't notice earlier. I've corrected them now.

github-actions[bot] commented 4 months ago

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/. You can also download an archive of the site from the workflow summary page which comes in handy when your dev site built was overriden by another PR (we have a single dev site!).

Azaya89 commented 4 months ago

Done!

github-actions[bot] commented 4 months ago

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/. You can also download an archive of the site from the workflow summary page which comes in handy when your dev site built was overriden by another PR (we have a single dev site!).