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

Further improvements to the demos #17

Closed
2 tasks
xhluca opened this issue Feb 2, 2021 · 21 comments · Fixed by #18
Closed
2 tasks

Further improvements to the demos #17

xhluca opened this issue Feb 2, 2021 · 21 comments · Fixed by #18

Comments

@xhluca
Copy link
Collaborator

xhluca commented Feb 2, 2021

Here are some improvements i'd like to make to the demos; @jourdain feel free to share your thoughts on them:

  • CFD: better seed line labels; it's not clear what the first and second sliders mean. I'll think of better labels
  • Slice Rendering: Remove the 3D volume viewer on the right-side to improve speed and make the app simpler/easier to understand.

@jourdain You mentioned about updating the sliders on change (instead of on release) for certain cases:
image

Could you remind me which ones you mentioned?

@jourdain
Copy link
Collaborator

jourdain commented Feb 2, 2021

Outputs that can be refreshed live (when sliding):

  • seed-lines
  • vtk-view

Outputs that can be refreshed at release / button press:

  • tube-mesh
  • tubes-rep
  • vtk-view
  • vtk-view

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

@jourdain Thanks for the list. I'll try to do that.

Something I noticed about usage-vtk-cfd is that the Viz class is mutated inside a callback. Moreover, since that object is defined on server-side when the app startups, there's a risk that two users using the app at the same time might interfere with each other by mutating the same object through their browser. A safer approach might be to create the object inside the callback:

def update_seeds(y1, y2, resolution, colorByField, presetName):
    viz = Viz(os.path.join(os.path.dirname(__file__), "data"))
    viz.updateSeedPoints(y1, y2, resolution)

    return [
        viz.getSeedState(),
        viz.getTubesMesh(colorByField),
        viz.getColorRange(),
        presetName,
        random.random(),  # trigger a render
    ]

Moreover, this approach is signfiicantly slower since we are re-reading the files in vtk every time. Is there a way of updating the visualization without mutating the Viz object? E.g. through deep copying or using some functional mechanism that would return a modified Viz object?

@jourdain
Copy link
Collaborator

jourdain commented Feb 2, 2021

If you know which user is which in a request/callback, just create and lookup a Viz per users.
That is the only way. In any case, sharing the same instance across users won't be an issue since the callback always compute a new output anyway.

The Viz object was there to streamline the usage and interaction of the app. Having things global would have lead to exactly the same issue (which is a none issue based on the architecture of dash).

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

I just tried to change how the Viz object is initialized by moving the expensive computations (loading bike_mesh and tunnelReader) inside the initial loading, such that we can safely and quickly create a new Viz object every time the callback is fired. Happy to hear your thoughts on doing that (now it only takes 0.01s to create a new object instead of ~0.18s).

See commit: a69ecaf

@jourdain
Copy link
Collaborator

jourdain commented Feb 2, 2021

What you've done is technically the same thing as sharing a Viz object across the users except that you keep creating and deleting VTK object in each request rather than reusing them.

1 similar comment
@jourdain
Copy link
Collaborator

jourdain commented Feb 2, 2021

What you've done is technically the same thing as sharing a Viz object across the users except that you keep creating and deleting VTK object in each request rather than reusing them.

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

In any case, sharing the same instance across users won't be an issue since the callback always compute a new output anyway.

That's good to know. I was initially worried about user 1 and user 2 simultaneously updating the seeds with their sliders (e.g. ms apart) but that shouldn't really be an issue since each worker will have its own instance of Viz and will only execute the request of one user at the time.

I've already committed a69ecaf but I'm happy to reverse it

@jourdain
Copy link
Collaborator

jourdain commented Feb 2, 2021

I would revert it, unless it is more readable for you.

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

What you've done is technically the same thing as sharing a Viz object

I'm not sure I understand; here I'm creating a new Viz object inside every callback:

def update_seeds(y1, y2, resolution, colorByField, presetName):
    viz = Viz(tunnelReader, bike_mesh)
    viz.updateSeedPoints(y1, y2, resolution)

    return [
        viz.getSeedState(),
        viz.getTubesMesh(colorByField),
        viz.getColorRange(),
        presetName,
        random.random(),  # trigger a render
    ]

Did you mean that the objects from the vtk module are shared?

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

I reverted the commit in 318a5ab

@jourdain
Copy link
Collaborator

jourdain commented Feb 2, 2021

No, what I mean is that creating 1 Viz object per user or sharing it across all the users will produce the same/valid result across the users. Basically there is no benefit of creating and deleting a Viz object.

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

I see, thank you

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

I was able to get the drag interaction to somewhat work:
cfd

Weird that i'm getting this error:
image

WIll spend some time investigating that

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

Seems like it happens when both sliders are close to -1, or both are close to 1

@jourdain
Copy link
Collaborator

jourdain commented Feb 2, 2021

The position of the slider should not matter.
That error could only happen if you are trying to fully execute the pipeline and get the tube geometry.

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

I'm not sure I understand. The error seems to also happen on master and 92186a6 / 067bcaa (before my demos refactoring):

cfd

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

If I set the line seeds to -1 in the initialization (rather than using callbacks):

class Viz():
    def __init__(self, data_directory):
        ...
        # Seeds settings
        self.resolution = 10
        self.point1 = [-0.4, -1, 0.05]
        self.point2 = [-0.4, -1, 1.5]

I seem to get this error stopping the app from booting:

(venv) (base) xhlu@XHL-Desktop:~/dev/dash-vtk$ python demos/usage-vtk-cfd/app.py 
Traceback (most recent call last):
  File "demos/usage-vtk-cfd/app.py", line 128, in <module>
    state=viz.getTubesMesh('p'),
  File "demos/usage-vtk-cfd/app.py", line 80, in getTubesMesh
    mesh_state = to_mesh_state(ds, color_by_field_name)
  File "/home/xhlu/dev/dash-vtk/dash_vtk/utils/vtk.py", line 37, in to_mesh_state
    points = vtk_to_numpy(polydata.GetPoints().GetData()).ravel()
AttributeError: 'NoneType' object has no attribute 'GetData'

@jourdain
Copy link
Collaborator

jourdain commented Feb 2, 2021

I see, I guess I clipped the domain in that region. What it means is that it doesn't have any geometry to generate. In other words polydata.GetPoints() == None.

@jourdain
Copy link
Collaborator

jourdain commented Feb 2, 2021

I should probably guard my converter for that, but for the demo, you could shrink the range to ensure that we will always have a streamline.

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

Sounds good!

@xhluca
Copy link
Collaborator Author

xhluca commented Feb 2, 2021

Thank you. I updated the sliders in 605a735 and there doesn't seem to be any other problems. If you are happy with that I'll merge the PR.

@xhluca xhluca closed this as completed in #18 Feb 3, 2021
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 a pull request may close this issue.

2 participants