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

Add dash-vtk chapter #1091

Merged
merged 72 commits into from
Apr 16, 2021
Merged

Add dash-vtk chapter #1091

merged 72 commits into from
Apr 16, 2021

Conversation

xhluca
Copy link

@xhluca xhluca commented Feb 4, 2021

Summary

I'm porting the docs written by @jourdain in the dash-vtk repo. I also added the dash examples.

  • Copied images from dash-vtk/docs/images to dash_docs/assets/images/vtk/
  • Added libgl1-mesa-glx and libXt to Aptfile since it is required by vtk for pip<=18
  • Added Dash VTK to dash_docs/chapter_index.py
  • Created a dash_docs/chapters/dash_vtk/ directory module and import it in dash_docs/chapter_index.py
  • Copied & modified the 7 examples from the dash-vtk/docs/tutorials to dash_docs/chapters/dash_vtk/examples
  • Ported the docs from dash-vtk/docs/README.md to dash_docs/chapters/dash_vtk/index.py; modified to dynamically display the demos in dash_docs/chapters/dash_vtk/examples
  • Created data file datasets/cow-nonormals.obj
  • Regenerated sitemap
  • Added dash-vtk==0.0.4, vtk==9.*;python_version>="3.7" and vtk==8.*;python_version<="3.6" to requirements.txt

Post-merge checklist:

The master branch is auto-deployed to dash.plotly.com.
Once you have merged your PR, wait 5-10 minutes and check dash.plotly.com
to verify that your changes have been made.

  • I understand

If this PR documents a new feature of Dash:

  • Comment on the original Dash issue with a link to the new docs.
  • Reply to any community thread(s) asking for this feature.

If this PR includes a new dataset available at a remote URL:

  • I have added this dataset to the datasets/ folder
  • I have added a mapping between the remote URL and the filename in the
    datasets/ folder into the find_and_replace dict in dash_docs/tools.py

If this PR adds an image or animated GIF:

  • This image was saved and referenced locally rather than via an external link

If I introduced a new relative link inside dcc.Markdown:

  • I considered whether I could replace the dcc.Markdown call with rc.Markdown, which will replace relative links with tools.relpath internally. Otherwise, I used e.g. <dccLink href=tools.relpath('/layout') children="the first chapter"/> instead of [the first chapter](/layout) (importing tools from dash_docs), and set dangerously_allow_html=true in the dcc.Markdown call.

If I changed the chapter_index by removing or relocating a page:

  • I added a redirect in dash_docs/server.py from the old URL to the new URL

@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 February 4, 2021 00:48 Inactive
@xhluca xhluca assigned xhluca and unassigned xhluca Feb 4, 2021
@xhluca
Copy link
Author

xhluca commented Feb 4, 2021

@jourdain If there's anything you'd like to modify, could you update dash_docs/chapters/dash_vtk/index.py directly? We can port them back later after this PR is merged.

@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 February 4, 2021 00:52 Inactive
@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 February 4, 2021 00:57 Inactive
Copy link

@jourdain jourdain left a comment

Choose a reason for hiding this comment

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

LGTM but also eager to see it live on your website

@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 February 4, 2021 15:36 Inactive
@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 February 4, 2021 15:45 Inactive
@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 February 4, 2021 15:47 Inactive
@xhluca
Copy link
Author

xhluca commented Feb 4, 2021

Seems like i'm getting this error in the tests:

――――――――――――――――――――――――――― test_sitemap_is_updated ――――――――――――――――――――――――――――

    @pytest.mark.skipif(sys.version_info < (3, 6), reason="requires python3.6 or higher")
    def test_sitemap_is_updated():
        from generate_sitemap import create_sitemap
        with open('dash_docs/assets/sitemap.xml', 'r') as f:
            saved_sitemap = f.read()
>       assert create_sitemap() == saved_sitemap
E       AssertionError: assert '<urlset xmln...\n</urlset>\n' == '<urlset xmln...\n</urlset>\n'
E         Skipping 20872 identical leading characters in diff, use -v to show
E           >
E           </url>
E         + <url>
E         +     <loc>https://dash.plotly.com/vtk</loc>
E         + </url>
E           </urlset>

tests/unit/test_sitemap.py:9: AssertionError

 tests/unit/test_sitemap.py ⨯                                    100% ██████████
=============================== warnings summary ===============================
dash_docs/convert_to_html.py:32
  /home/circleci/project/dash_docs/convert_to_html.py:32: DeprecationWarning:
  
  invalid escape sequence \s

and it's preventing the heroku app from booting. Any idea about the cause @plotly/customer-success ? Regenerating sitemap

@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 April 14, 2021 05:13 Inactive
@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 April 14, 2021 05:21 Inactive
@jdamiba
Copy link
Contributor

jdamiba commented Apr 14, 2021

@xhlulu Just a reminder that we will need to edit the requirements.txt file here to point to the the PyPi version of dash-vtk instead of a reference to GitHub once the lazy-loading branch has been merged to master and a new release of dash-vtk has been created.

@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 April 14, 2021 16:15 Inactive
@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 April 14, 2021 17:26 Inactive
@xhluca xhluca temporarily deployed to dash-docs-pr-1091 April 15, 2021 21:02 Inactive
@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 April 16, 2021 18:58 Inactive
@jdamiba
Copy link
Contributor

jdamiba commented Apr 16, 2021

This PR looks good to me 💃🏿

I tested that the async functionality works on the Heroku review app.

Copy link
Contributor

@rpkyle rpkyle left a comment

Choose a reason for hiding this comment

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

Just a few minor comments, otherwise this looks OK to me.

dash_docs/chapter_index.py Outdated Show resolved Hide resolved
content = dash_vtk.View([
dash_vtk.VolumeRepresentation([
# GUI to control Volume Rendering
# + Setup good default at startup
Copy link
Contributor

Choose a reason for hiding this comment

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

    # + Setup good default at startup

Is it possible to elaborate/clarify why this step is necessary? Is it because the default values are not provided unless VolumeController() is invoked? Or does it modify the existing defaults when called?

Copy link
Author

Choose a reason for hiding this comment

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

@jourdain ☝️

Copy link

@jourdain jourdain Apr 16, 2021

Choose a reason for hiding this comment

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

The values get computed based on the volume data and geometry. While the default values are static and therefore will have little chance to produce a good result across a various set of volumes.

Normally an advanced user will update the parameters the way he wants the volume to be rendered. But if you don't know what the data is about, using the controller would give you a reasonable first rendering (on top of being dynamically tunable). We could recreate the computation of those values inside the volume representation but it will have to be under a property like viz-profile="auto" or something equivalent.

Copy link

@jourdain jourdain Apr 16, 2021

Choose a reason for hiding this comment

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

The magic value computations is happening here

import dash_html_components as html

import dash_vtk

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we missing the compatibility block used above here?

try:
   # VTK 9+
   from vtkmodules.vtkImagingCore import vtkRTAnalyticSource
except:
  # VTK =< 8
  from vtk.vtkImagingCore import vtkRTAnalyticSource

Copy link
Author

Choose a reason for hiding this comment

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

@rpkyle that's handled internally by dash_vtk I believe

Copy link
Contributor

Choose a reason for hiding this comment

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

@rpkyle Note that this example piece of code is importing from the vtkmodules library, not dash_vtk.

No compatibility check needed if the import is not from vtkmodules.

Copy link

@jourdain jourdain Apr 16, 2021

Choose a reason for hiding this comment

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

vtkmodules is only available for 9+, so the check should be made where the import happen...

Any version will work if using vtk.xxx but doing so will load the full library rather than just the required subset.

import dash_html_components as html

import dash_vtk

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about the VTK 9+ vs. earlier versions codeblock used above but missing here. Should we add it for this example and any others that follow?

Copy link
Author

Choose a reason for hiding this comment

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

Same response as above.

@@ -48,7 +48,7 @@ def test_page_menu_001(dash_doc):
dash_doc.wait_for_element_by_id("page-menu--links")

home_links = [
'Dash User Guide',
'Dash Python User Guide',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'Dash Python User Guide',
'Dash for Python User Guide',

Copy link
Contributor

Choose a reason for hiding this comment

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

This change is outside of the scope of this PR- if we want to reword the heading let's do it in a separate PR where we change it both here in the test and in the markup so that CI keeps passing.

Co-authored-by: Ryan Patrick Kyle <rpkyle@users.noreply.github.com>
@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 April 16, 2021 20:05 Inactive
Co-authored-by: Ryan Patrick Kyle <rpkyle@users.noreply.github.com>
@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 April 16, 2021 20:05 Inactive
Co-authored-by: Ryan Patrick Kyle <rpkyle@users.noreply.github.com>
@chriddyp chriddyp temporarily deployed to dash-docs-pr-1091 April 16, 2021 20:07 Inactive
@rpkyle rpkyle self-requested a review April 16, 2021 20:15
@rpkyle
Copy link
Contributor

rpkyle commented Apr 16, 2021

Once we can get the tests passing, 💃

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.

None yet

9 participants