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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(vis): violin vis selection events #267

Merged
merged 11 commits into from
Apr 24, 2024

Conversation

dvmartinweigl
Copy link
Contributor

@dvmartinweigl dvmartinweigl commented Apr 23, 2024

Partially fixed: #227

Developer Checklist (Definition of Done)

Issue

  • All acceptance criteria from the issue are met
  • Tested in latest Chrome/Firefox

UI/UX/Vis

  • Requires UI/UX/Vis review
    • Reviewer(s) are notified (tag assignees)
    • Review has occurred (link to notes)
    • Feedback is included in this PR
    • Reviewer(s) approve of concept and design

Code

  • Branch is up-to-date with the branch to be merged with, i.e., develop
  • Code is cleaned up and formatted
  • Unit tests are written (frontend/backend if applicable)
  • Integration tests are written (if applicable)

PR

  • Descriptive title for this pull request is provided (will be used for release notes later)
  • Reviewer and assignees are defined
  • Add type label (e.g., bug, feature) to this pull request
  • Add release label (e.g., release: minor) to this PR following semver
  • The PR is connected to the corresponding issue (via Closes #...)
  • Summary of changes is written

Summary of changes

  • fixed selection events in violin plot not working properly
  • disabled zoom for violin plot
  • fixed: vis configurations not applied in storybook

Screenshots

Additional notes for the reviewer(s)

Thanks for creating this pull request 馃

@dvmartinweigl dvmartinweigl added the type: bug Something isn't working label Apr 23, 2024
@dvmartinweigl dvmartinweigl self-assigned this Apr 23, 2024
@dvmartinweigl dvmartinweigl changed the title Mweigl/add proper violin plot selection Fix violin vis selection events Apr 23, 2024
@dvmartinweigl dvmartinweigl changed the title Fix violin vis selection events Vis: Fix violin vis selection events Apr 23, 2024
@dvdanielamoitzi
Copy link
Contributor

Thanks for this PR @dvmartinweigl! I have tested it in BioInsight, and for me it works as expected 馃憤

@thinkh thinkh changed the title Vis: Fix violin vis selection events fix(vis): violin vis selection events Apr 24, 2024
@dvmartinweigl dvmartinweigl marked this pull request as ready for review April 24, 2024 08:15
thinkh
thinkh previously requested changes Apr 24, 2024
Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

I tested this PR in Ordino and it does not work from the first look.

Steps to reproduce

  1. Open Cell line workbench
  2. Add Micro Satellite Instability (MSI) score
  3. Open Configurable charts view
  4. Switch to violin plot
  5. Select Gender as categorical column

Observations

Ordino DEV

grafik

  • The violin plot has colors (even though they don't match the gender categorical colors)
  • Tooltips on hover are working
  • Selecting the male violin plot selects rows in the data table BUT it selects also female rows (which is clearly wrong) --> no highlight of the male violin plot
    grafik
  • Selection in the data table is not reflected in the violin plot (i.e., no highlight)

Local version (with this PR)

grafik

Shows the following error in the dev console on hover:

Uncaught TypeError: Cannot read properties of undefined (reading '0')
    at _ (plotly.min.js:37:1)
    at O (plotly.min.js:37:1)
    at r.getClosest (plotly.min.js:37:1)
    at Object.l [as hoverOnBoxes] (plotly.min.js:37:1)
    at e1.exports [as hoverPoints] (plotly.min.js:37:1)
    at ut (plotly.min.js:37:1)
    at plotly.min.js:37:1
    at plotly.min.js:37:1
    at l (plotly.min.js:37:1)
    at r.throttle (plotly.min.js:37:1)
  • No colored violin plots
  • No tooltips on hover -> instead error above
  • Selecting a violin plot has no effect but repeating the error message
  • Selecting some rows in the data table always highlights the first violin plot (independent if only male or female or mixed are selected)
    grafik

Expected result

The error should be fixed and the selection must work again. In best case the categorical colors are also matching the one in the LineUp.


Local dev setup

@dvchristianbors Steps to run the setup due to the missing merge and release of datavisyn/visyn_scripts#67.

package.json resolutions

{
"resolutions": {
    "visyn_core": "git+ssh://[email protected]/datavisyn/visyn_core.git#mweigl/add-proper-violin-plot-selection",
    "visyn_scripts": "git+ssh://[email protected]/datavisyn/visyn_scripts.git#mweigl/remove-security-routes-proxy"
}
}

requirements.txt --> add the following two dependencies:

visyn_core@git+https://github.com/datavisyn/visyn_core.git@develop#egg=visyn_core
# pin werkzeug version because the new major version 3.0.0 breaks our applications on Oct 2 2023; the version can be removed later again when the other packages support v3.0.0
werkzeug==2.3.7

mock_data_api/requirements.txt

uvicorn[standard]==0.29.0
fastapi==0.110.1

Run yarn install and make develop.

Copy link
Contributor

@dvchristianbors dvchristianbors left a comment

Choose a reason for hiding this comment

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

Thank you for your changes.

I reviewed @dvmartinweigl 's latest changes:

No colored violin plots

Will be covered in a follow-up PR that also addresses synchronizing colors between a possible ranking and the vis.

No tooltips on hover -> instead error above

Error no longer occurs

Selecting a violin plot has no effect but repeating the error message

Selection now works (also multi-select)

Selecting some rows in the data table always highlights the first violin plot (independent if only male or female or mixed are selected)

Selections in the ranking will highlight the correct violins, accordingly.

Another comment from my side:
As soon as there is a large number of violin plots, the individual plots no longer can be hovered or selected. However, this seems to be a limitation within plotly itself, and is only really solvable by the user specifically filtering out categories. I think this is fine, since for a general vis, we can not realistically cover all edge cases.

@thinkh thinkh dismissed their stale review April 24, 2024 18:58

@dvchristianbors tested the most recent changes on behalf of me

@thinkh thinkh merged commit 7c4adcf into develop Apr 24, 2024
4 checks passed
@thinkh thinkh deleted the mweigl/add-proper-violin-plot-selection branch April 24, 2024 19:03
@dvvanessastoiber dvvanessastoiber mentioned this pull request Apr 25, 2024
puehringer added a commit that referenced this pull request Apr 25, 2024
## What's changed

* feat: add possibility to insert score column after given column in
ranking (#262)
* refactor: rename labels of filter buttons
(#266)
* fix(vis): duplicate option values in scatterplot sidebar select
(#261)
* chore: remove usage of flask
(#257)
* fix(vis): violin vis selection events
(#267)
@thinkh thinkh mentioned this pull request May 2, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bugs related to Vis
4 participants