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

Refactor manager internal API, speed up string filters in UI, add manager API tests #1077

Merged
merged 8 commits into from
Jun 24, 2020

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Jun 4, 2020

This PR reduces the public API/code footprint of PerspectiveManager by moving all internal logic to a separate base class, which should make the Manager API more "manageable" and easier to read. Additionally, it reduces the amount of computation required for string filters by making the suggestions list quicker to compute and serialize.

Python Changelog

  • Add PerspectiveManagerInternal, which hosts all internal methods that maintain the wire API.
  • Add tests around PerspectiveSession being able to open/close multiple sessions
  • Add tests around PerspectiveManager handling on_update and remove_update over the wire API
  • __ROW_PATH__ values are now python values that match the underlying schema (datetime.datetime for "datetime" columns, etc.) instead of strings. This behavior is now consistent with perspective.js.
  • Makes _PerspectiveCallbackCache easier to use by removing the need to get the underlying list before operating on its contents
  • Changes _PerspectiveCallbackCache.remove_callback to remove the callbacks for which the lambda returns True. This changes its behavior and makes it no longer consistent with filterInPlace, which is used in the Javascript version of Perspective to remove callbacks.
  • Removes numpy.isnat and fixes several pandas and numpy tests around datetimes
  • Replaces isinstance(str) with isinstance(six.string_types) for Python 2 compatibility

JS Changelog

  • Makes filter suggestions for string columns faster by a magnitude of ~10x (on large datasets)
    • Because we don't use the actual columns of the dataset to generate the suggestion, make the new view with columns=[] instead of serializing all columns and immediately disposing of them.

@sc1f sc1f requested a review from timkpaine June 4, 2020 22:41
@sc1f sc1f added enhancement Feature requests or improvements internal Internal refactoring and code quality improvement JS Python and removed enhancement Feature requests or improvements labels Jun 4, 2020
@sc1f
Copy link
Contributor Author

sc1f commented Jun 16, 2020

@texodus this branch is rebased on top of #1082 - please merge that PR first so we have consistent line endings, otherwise the eol=lf in .gitattributes is preventing me from most git operations because line endings are automatically changed, even if I discard those changes in the working directory

@sc1f sc1f removed the request for review from timkpaine June 16, 2020 22:10
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Looks good! Nice catch on the autocomplete performance bug!

@texodus texodus merged commit 1abd35d into master Jun 24, 2020
@texodus texodus deleted the manager-fix branch June 24, 2020 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Internal refactoring and code quality improvement JS Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants