feat(maidr.show): support py-shiny renderer #67
Conversation
|
@krishnanand5 I think you have accidentally closed this PR. Please reopen. |
|
Greetings Professor, In an attempt to resolve a merge conflict with poetry.lock, I synced my fork and it caused this unforeseen change. I am rectifying my local repo and will be making a commit shortly with the updated scripts. Regards, |
|
Aw, I see. Thanks for letting me know!
|
|
Greetings Professor, I have addressed all the initial concerns in the respective sections and would like to discuss one change that I have implemented in the As shiny provides complete freedom to the user to define the cosmetics of a component that they are creating, the same applies to plot objects as well. In order to view a maidr plot, we require the component area to be slightly bigger so that the user can experience the interactiveness without having to use scrollbars - in other words, overflow of the web component is a factor to consider/document. For now, I have modified the render to expect a width and height parameter during function invocation. If you feel we should handle this in a better way, please do suggest any measures and I will begin working on them. I am investigating Regards, |
|
@SaaiVenkat and @dakshpokar -- Please review this PR as well. |
|
I will merge this PR after #64 is fully addressed. |
|
|
||
| def wrapper( | ||
| *args: Any, | ||
| width: Optional[str] = None, |
There was a problem hiding this comment.
Hi @krishnanand5,
Can you automatically resize the iframe if height and width are not passed as arguments? Please feel free to refer to #64 for guidance. Considering this library's target audience, we should try to ensure the interface is as user-friendly and accessible as possible.
Apart from that the PR looks clean, good work!
As for the comment that you left on #64 - My observation is that Python notebooks typically have a considerable level of flexibility when it comes to handling cell rendering, which makes suitable style changes possible.
To handle this, you may want to try utilizing Inline-JS/CSS to more accurately regulate the overflow behavior.
There was a problem hiding this comment.
- Since iframe seems to addressed here as well, I would suggest to merge this PR after feat(maidr.show): support interactive maidr plots within Jupyter Notebooks, VS Code, Google Colab, and Quarto #64 .
- Also, please create a package called 'widget' under 'maidr' package (maidr/widget/shiny.py), and move the contents there. We'll be extending this further to support GUI widgets in the future.
- We are using the htmltools ui underneath, and shiny uses the same. This annotation duplicates many of the functionalities from shiny as well as from htmltools. This could be simplified by the following
from __future__ import annotations
from shiny.render import ui
from shiny.types import Jsonifiable
import maidr
class render_maidr(ui):
async def render(self) -> Jsonifiable:
value = await self.fn()
if value is None:
return None
value = maidr.render(value)
rendered = await self.transform(value)
return rendered
There was a problem hiding this comment.
The requested changes have been addressed in in this commit.
|
|
||
| def wrapper( | ||
| *args: Any, | ||
| width: Optional[str] = None, |
There was a problem hiding this comment.
- Since iframe seems to addressed here as well, I would suggest to merge this PR after feat(maidr.show): support interactive maidr plots within Jupyter Notebooks, VS Code, Google Colab, and Quarto #64 .
- Also, please create a package called 'widget' under 'maidr' package (maidr/widget/shiny.py), and move the contents there. We'll be extending this further to support GUI widgets in the future.
- We are using the htmltools ui underneath, and shiny uses the same. This annotation duplicates many of the functionalities from shiny as well as from htmltools. This could be simplified by the following
from __future__ import annotations
from shiny.render import ui
from shiny.types import Jsonifiable
import maidr
class render_maidr(ui):
async def render(self) -> Jsonifiable:
value = await self.fn()
if value is None:
return None
value = maidr.render(value)
rendered = await self.transform(value)
return rendered
maidr/maidr.py
Outdated
| @@ -10,6 +11,14 @@ | |||
| from maidr.core.figure_manager import FigureManager | |||
|
|
|||
|
|
|||
| def render( | |||
| plot: Any, *, lib_prefix: str | None = "lib", include_version: bool = True | |||
| ) -> RenderedHTML: | |||
There was a problem hiding this comment.
In order to incorporate the simplified rendering we can return the Tag object instead of the RenderedHTML.
def render(plot: Any) -> Tag:
ax = FigureManager.get_axes(plot)
maidr = FigureManager.get_maidr(ax.get_figure())
return maidr.render()
And update core/maidr.py to render the Tag
def render(self) -> Tag:
return self._create_html_tag()
There was a problem hiding this comment.
Thank you @SaaiVenkat, for these suggestions. These changes have been incorporated and have made the module more compact
|
|
||
| # Define the server | ||
| def server(input, output, session): | ||
| @reactive.Calc |
There was a problem hiding this comment.
reactive.Calc is mainly used to perform mathematical intensive calculations. May I know why this has been used here?
There was a problem hiding this comment.
Yes, reactive.calc is residual from development. Will remove this from the example
| return s_plot | ||
|
|
||
| @output | ||
| @render.ui |
There was a problem hiding this comment.
This plot has been injected into the shiny through ui.output_ui. Why @output is required?
There was a problem hiding this comment.
@output is for rendering reactive output on the UI but since @render.ui is sufficient in our case to render the generated HTML on the shiny app, @output is redundant. Will have this removed.
| @output | ||
| @render.ui | ||
| def reactivebarplot(): | ||
| return render_maidr(create_reactivebarplot)(width="100%", height="700px") |
There was a problem hiding this comment.
Could @render_maidr be used to render the example? If so, please update this to reflect the same.
There was a problem hiding this comment.
As @render_maidr is a decorator, we can invoke this directly.
|
Greetings @SaaiVenkat, Following are the list of changes I have done in the recent commits:
I did check possible collateral effects of removing the conditional check but turns out only the interactive python module and shiny module are currently leveraging it for iframes. Please let me know if the changes made are as per expectations and if there are any further changes you would like on this module. Regards, |
| import maidr | ||
|
|
||
|
|
||
| class render_maidr(ui): |
There was a problem hiding this comment.
We use PascalCase for class name in our package. Why do you use snake_case here? Is this the Shiny convention?
There was a problem hiding this comment.
I checked Shiny and there are no such conventions. I have renamed the class name to adhere to PascalCase.
maidr/widget/shiny.py
Outdated
| import maidr | ||
|
|
||
|
|
||
| class RenderMaidr(ui): |
There was a problem hiding this comment.
@krishnanand5 shiny breaks the conventional Pascal case for classes, since this will be used as a decorator, like @render_maidr. There have followe this convention for all their renderers. So, we should also follow their convention.
cc: @jooyoungseo
There was a problem hiding this comment.
Thank you for the confirmation @SaaiVenkat. I thought our repository's naming convention superseded supporting libraries but you are right. Better to follow the naming convention of the supporting libraries. I have reverted the class name to snake_case
This PR adds support for maidr on py-shiny through a custom renderer called
render_maidr. This decorator internally calls the newly createdmaidr.renderfunction to transform a plot of the matplotlib type to a maidr object and envelops it as an iframe srcdoc for display on a py-shiny web dashboard.Closes [#65]