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

Lazy scatterplot for Vector & Table #3655

Merged
merged 13 commits into from
Aug 23, 2022

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Aug 18, 2022

Pull Request Description

First of all this PR demonstrates how to implement lazy visualization:

  • one needs to write/enhance Enso visualization libraries - this PR adds two optional parameters (bounds and limit) to process_to_json_text function.
  • the process_to_json_text can be tested by standard Enso test harness which this PR also does
  • then one has to modify JavaScript on the IDE side to construct setPreprocessor expression using the optional parameters

The idea of scatter plot lazy visualization is to limit the amount of points the IDE requests. Initially the limit is set to limit=1024. The Scatter_Plot.enso then processes the data and selects/generates the limit subset. Right now it includes min, max in both x, y axis plus randomly chosen points up to the limit.

Zooming In

The D3 visualization widget is capable of zooming in. When that happens the JavaScript widget composes new expression with bounds set to the newly visible area. By calling setPreprocessor the engine recomputes the visualization data, filters out any data outside of the bounds and selects another limit points from the new data. The IDE visualization then updates itself to display these more detailed data. Users can zoom-in to see the smallest detail where the number of points gets bellow limit or they can select Fit all to see all the data without any bounds.

Important Notes

Randomly selecting limit samples from the dataset may be misleading. Probably implementing k-means clustering (where k=limit) would generate more representative approximation.

Checklist

Please include the following checklist in your PR:

  • Implement the same algorithm for Table and Column
  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

Reviewed the JS part only!

Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Looks a reasonable approach - one concern is how to allow loading of more points after the first 32.

@@ -58,6 +61,14 @@ class ScatterPlot extends Visualization {
this.points = { labels: VISIBLE_POINTS }
}

updatePreprocessor() {
let fn = 'x -> process_to_json_text x limit=' + this.limit
Copy link
Member Author

@JaroslavTulach JaroslavTulach Aug 19, 2022

Choose a reason for hiding this comment

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

CCing @4e6: Calling this.setPreprocessor repeatedly with different code snippet seems to work, but ideally we shall have a single snippet and just invoke it with different limit and bounds argument. I hope it will be possible to do something like that with hidden modules.

#3661 introduces this.setPreprocessor(module, method) - it shall also allow providing arguments somehow.

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Aug 19, 2022
@JaroslavTulach JaroslavTulach changed the title Enhanced visualization support for Vector, its test and usage from IDE Lazy scatterplot for Vector & Table Aug 20, 2022
Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

One slight change - just want to think if an easier way to do the extremes.

@mergify mergify bot merged commit 2b9352d into develop Aug 23, 2022
@mergify mergify bot deleted the wip/jtulach/LazyScatterPlot_182860818 branch August 23, 2022 12:12
@JaroslavTulach
Copy link
Member Author

Thanks James for the 6473ad1 simplification of the computation of the sample.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants