Skip to content

Conversation

xhluca
Copy link

@xhluca xhluca commented Feb 3, 2021

Wow the user guide is really great! Just minor details:

  • Some parts include react code (see example). Should we modify them to be Dash code?
  • I'm not sure whether it's possible to import those modules since they start with numbers. Will think about it but might need to rename them if I can't use importlib; would that be ok?

@jourdain
Copy link
Contributor

jourdain commented Feb 3, 2021

  1. The react snippet is to illustrate the alias those component represent and how their property get mapped to the underlying structure. The purpose was to show that is a short hand of what they would have to do in case they were using the core components. I'm not sure the dash syntax would be easier to read. Let me know if you think those snippet are valuable to the user and if it would be better to convert them to python snippet? The idea was to provide pseudo code so they understood the idea behind those simpler components.

  2. Those are actual working examples that are referenced and listed in that readme. I tested them by simply running python 00_xxxx.py. The idea behind the number is really to order them in term of progression based on when they appear in the README.

Copy link
Contributor

@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.

Thanks for fixing typos

@xhluca
Copy link
Author

xhluca commented Feb 3, 2021

Those are actual working examples that are referenced and listed in that readme. I tested them by simply running python 00_xxxx.py. The idea behind the number is really to order them in term of progression based on when they appear in the README.

Sorry, I meant I might have some trouble importing them i.e.

import 00_...

But I think I can just use that approach.

@xhluca
Copy link
Author

xhluca commented Feb 3, 2021

Let me know if you think those snippet are valuable to the user and if it would be better to convert them to python snippet?

I think it's a good idea keeping the snippets, I'm mostly worried that they will confuse the dash users that only know Python but not JSX.

how their property get mapped to the underlying structure.

I'm not sure I fully understand that. Do you mean the react property or the properties parameters in Python/Dash?

@jourdain
Copy link
Contributor

jourdain commented Feb 3, 2021

I'm not sure I fully understand that. Do you mean the react property or the properties parameters in Python/Dash?

See https://github.com/Kitware/react-vtk-js/blob/master/src/representations/VolumeDataRepresentation.js#L41-L69

Those are just React alias to some degree.

number in app modules

We can prefix them with tuto_{number}_{name}.py

@jourdain
Copy link
Contributor

jourdain commented Feb 3, 2021

I'll merge your fix and take a stab at your comments.

@jourdain jourdain merged commit 1ec4007 into master Feb 3, 2021
@xhluca
Copy link
Author

xhluca commented Feb 3, 2021

@jourdain thank you!

@jourdain jourdain deleted the review-docs branch February 3, 2021 22:51
@jourdain
Copy link
Contributor

jourdain commented Feb 3, 2021

Would the following be easier for your users?

def PointCloudRepresentation(**kwargs):
  return dash_vtk.GeometryRepresentation(
    id=kwargs.get('id'),
    colorMapPreset=kwargs.get('colorMapPreset'),
    colorDataRange=kwargs.get('colorDataRange'),
    property=kwargs.get('property'),
    children=[
      dash_vtk.PolyData(
        points=kwargs.get('xyz'),
        connectivity='points',
        children=[
          dash_vtk.PointData([
            dash_vtk.DataArray(
              registration='setScalars',
              values={kwargs.get('scalars')}
            )
          ])
        ],
      )
    ],
  )

@jourdain
Copy link
Contributor

jourdain commented Feb 3, 2021

Pushed all the updates... Feel free to give another look.

@jlevy44
Copy link

jlevy44 commented Jan 16, 2022

@jourdain I am still getting this issue with the PointCloudRepresentation, where the ID is found, but still some "id" issue:

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-22-c477e139820c> in <module>
     37 
     38             ],
---> 39         ) for i in [-0.5,0.5]]+
     40         [dash_vtk.GeometryRepresentation(
     41             id=f"blueline-{i}",

~/.local/lib/python3.7/site-packages/dash/development/base_component.py in wrapper(*args, **kwargs)
    364         if "self" in kwargs["_explicit_args"]:
    365             kwargs["_explicit_args"].remove("self")
--> 366         return func(*args, **kwargs)
    367 
    368     # If Python 3, we can set the function signature to be correct

~/.local/lib/python3.7/site-packages/dash_vtk/PointCloudRepresentation.py in __init__(self, xyz, rgb, rgba, scalars, colorMapPreset, colorDataRange, property, **kwargs)
     50                 raise TypeError(
     51                     'Required argument `' + k + '` was not specified.')
---> 52         super(PointCloudRepresentation, self).__init__(**args)

~/.local/lib/python3.7/site-packages/dash/development/base_component.py in __init__(self, **kwargs)
    110                     )
    111                     + "\nAllowed arguments: {}".format(  # pylint: disable=no-member
--> 112                         ", ".join(sorted(self._prop_names))
    113                     )
    114                 )

TypeError: The `dash_vtk.PointCloudRepresentation` component (version 0.0.9) with the ID "model" received an unexpected keyword argument: `id`
Allowed arguments: colorDataRange, colorMapPreset, property, rgb, rgba, scalars, xyz

@jourdain
Copy link
Contributor

569c1f4

@jourdain
Copy link
Contributor

jourdain commented Jan 17, 2022

@xhlulu , you may want to do a pass when you update react-vtk-js and vtk.js and publish a new dash-vtk version.

@xhluca
Copy link
Author

xhluca commented Jan 17, 2022

Awesome work @jourdain! @alexcjohnson could you forward that to the current maintainer of dash-vtk?

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.

3 participants