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

feat(vis-type: scatter): improve rendering of scatter plot #530

Merged
merged 59 commits into from
Oct 16, 2024

Conversation

dvmoritzschoefl
Copy link
Contributor

@dvmoritzschoefl dvmoritzschoefl commented Sep 19, 2024

Closes #522 ,https://github.com/datavisyn/reprovisyn/issues/1870, https://github.com/datavisyn/communication_bi_ordino/issues/76, https://github.com/datavisyn/communication_bi_ordino/issues/77,
https://github.com/datavisyn/reprovisyn/issues/2028,
https://github.com/datavisyn/communication_bi_ordino/issues/79

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

  • fix regression stats

Screenshots

Additional notes for the reviewer(s)

Thanks for creating this pull request 🤗

@dvmoritzschoefl dvmoritzschoefl marked this pull request as ready for review September 23, 2024 15:02
@dvmoritzschoefl dvmoritzschoefl requested a review from a team as a code owner September 23, 2024 15:02
@thinkh
Copy link
Member

thinkh commented Oct 10, 2024

@dvmoritzschoefl as discussed in call:

  1. Distribute subplots equally for the available width
    image

  2. Changing the drag mode in the vis config is not reflected in the plot

@dvmartinweigl
Copy link
Contributor

Could we maybe expose a prop in the scattervis config to change the type of splom.
What do you think @dvmoritzschoefl

Something like this:
splomMode:

  • lowerTriangle
  • fullMatrix
  • hideDiagonal
  • histogram (if added in future)

@dvmoritzschoefl
Copy link
Contributor Author

Could we maybe expose a prop in the scattervis config to change the type of splom. What do you think @dvmoritzschoefl

Something like this: splomMode:

  • lowerTriangle
  • fullMatrix
  • hideDiagonal
  • histogram (if added in future)

I would not make it more complicated than needed for now. I think if we cannot have histograms, then the lower triangle mode is the best bet (and also the best performing one). I would add it if someone needs this at some point.

@dvmoritzschoefl
Copy link
Contributor Author

@dvmoritzschoefl as discussed in call:

  1. Distribute subplots equally for the available width
    image
  2. Changing the drag mode in the vis config is not reflected in the plot

The number of columns and rows is now calculated by the following rule:

  • per 400 pixels one more columns comes (until a maximum of 4 columns) -> if less facets than columns are present then the number of columns is the same as the number of facets
  • the number of rows is facetnumber / columnnumber
    while not perfect for sure this works well for the 1-10 facet cases

as for the plot not updating and the dragmode having no effect. i think this is an ordino problem (as discussed in call). i tried it in the storybook and when using the config in an external state and updating there it works as expected

@dvchristianbors
Copy link
Contributor

dvchristianbors commented Oct 14, 2024

One more small remark is regarding the legend and coloring of unknown categorical values

image

The categories are not 'sorted' (previously, the 'Unknown' category always came last), and the vis_neutral_color is not used, as recommended:

/**
 * Neutral color (e.g., histogram in scatterplot matrix and should be also used for "Unknown" categorical values)
 */
export const VIS_NEUTRAL_COLOR = '#71787E';

https://github.com/datavisyn/visyn_core/blob/a2d4450bd2eb9a5d86d84fb4ac99fe0ba1c5c15c/src/vis/general/constants.ts

#71787E

@dvmartinweigl
Copy link
Contributor

Something seems wrong with the stats calculations. Adding 4 numeric columns (all gene expression) to the config results in the last row of the lower triangle having NaN values for pValue, r² ....
Could it be that there is an error in the data getting passed to fitRegressionLine @dvmoritzschoefl ?

image

puehringer
puehringer previously approved these changes Oct 16, 2024
thinkh
thinkh previously approved these changes Oct 16, 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.

Thanks for the performance improvements and addressing all the various feedback. We have tested it in multiple apps and it is ready to merge.

@thinkh
Copy link
Member

thinkh commented Oct 16, 2024

@dvmoritzschoefl I saw that the Playwright tests are failing for this branch. in comparison the develop branch succeeded with the latest merge. Please check the tests and fix them. Thanks.

@thinkh thinkh dismissed stale reviews from puehringer and themself via ce8d75b October 16, 2024 18:25
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 fixed the failing Playwright tests by adding and using the correct selectors for the legend.

@thinkh
Copy link
Member

thinkh commented Oct 16, 2024

The download Playwright tests are currently timing-out but all other tests are working as expected.

@thinkh thinkh merged commit 913d5ea into develop Oct 16, 2024
7 of 8 checks passed
@thinkh thinkh deleted the mh/scatterplot_new branch October 16, 2024 19:46
@github-actions github-actions bot mentioned this pull request Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: minor PR merge results in a new minor version type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EPIC: Scatter plot issues
6 participants