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

[ENH] Plot histogram for a user-selected column #97

Merged
merged 24 commits into from
Oct 18, 2023

Conversation

alyssadai
Copy link
Collaborator

@alyssadai alyssadai commented Oct 12, 2023

Closes #82

Changes proposed in this pull request:

  • Add dropdown for user to select a datatable column to plot when input is phenotypic
  • Add graph component and plotting function for phenotypic column histogram
  • Add helper function to wrap very long string values in columns to avoid long histogram tick labels
  • Make histogram responsive to datatable filtering
  • Update tests:
    • Smoke test for a valid phenotypic bagel now looks for presence of the phenotypic col dropdown
    • Add test that a relevant histogram is displayed when a column is selected from the dropdown to plot
  • Remove unused extra None return value for callback, which was missed in [FEAT] Add schema, upload route, + callback logic to parse a phenotypic bagel #71 (not sure why this wasn't causing any errors??)
  • More informative comments added

Checklist

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see https://neurobagel.org/contributing/pull_requests for more info)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

@alyssadai alyssadai requested a review from nikhil153 October 12, 2023 14:47
@alyssadai
Copy link
Collaborator Author

alyssadai commented Oct 12, 2023

Hey @nikhil153, looping you in here for a functional review of this new dashboard feature if you have the availability (as before, Arman or Seb will do the code review). Feel free to also to try it out once the PR is merged and let me know if you have any feedback on the usability.

(FYI, the other features we discussed - stratifying the histogram by session, a summary stats box - are tracked separately to avoid a massive PR here #88)

@nikhil153
Copy link
Contributor

Thanks @alyssadai!

Overall it works great! Minor changes would be:

  1. Make histogram style to have separate bars with spaces between to clearly represent bin centers.
    image
  2. This might be related to screen sizes / browser, but when first loaded, I could not see the Select a column to plot: title until I scrolled down. If it's easy, we should rearrange the layout, so that
  • Plotting functionality is seen right away
  • Plotting title has bigger font and should say what it is getting plotted (e.g. count distribution)
  • Plot itself need not cover the entire width of the screen.
  1. Also if it's simple enough, we should add along with the plot
  • counts for missing values
  • mean and std-dev for the available (i.e. plotted) values.

@alyssadai
Copy link
Collaborator Author

alyssadai commented Oct 12, 2023

Thanks for the prompt feedback @nikhil153!

  1. Make histogram style to have separate bars with spaces between to clearly represent bin centers.
  • Okay, will update histogram so that there are spaces between all bars regardless of whether the x-values are categorical/continuous 👍
  1. This might be related to screen sizes / browser, but when first loaded, I could not see the Select a column to plot: title until I scrolled down.

Yeah, this will unfortunately vary according to screen and browser size + zoom so is challenging to standardize. What I can do is

  • Make the plotting title font bigger (and also make the title itself more informative as suggested)
  • Try and move the column plotting dropdown onto the same row as the "Advanced filtering options" dropdown (this is not currently the arrangement because of the varying numbers of dropdowns for "Advanced filtering options" between imaging and phenotypic dashboard layouts)
  • ^if this doesn't work well, I'll try to at least move the column plotting dropdown and plot onto the same row so each is taking up less width-space, and I'll add a tip somewhere higher up in the dashboard about the plotting function below

Note that even with the above changes, with a narrow/small enough browser elements will still inevitably wrap to the next row.

  1. Also if it's simple enough, we should add along with the plot
    counts for missing values
    mean and std-dev for the available (i.e. plotted) values.

I'll add these separately as part of addressing #96 to make the PR code reviews easier 🙂

@alyssadai
Copy link
Collaborator Author

@nikhil153, I've implemented your feedback regarding the histogram plot and dropdown.

Mind quickly testing out the dashboard again to let me know if the updated layout looks good? 🙂 A quick note, I shortened the width of the histogram as suggested, but have left it left-justified for now since a box with the summary stats will be added next to it soon.

@nikhil153
Copy link
Contributor

@alyssadai - Looks great! Thanks :)

@surchs surchs self-requested a review October 18, 2023 00:40
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Very cool additions, and impressive tests! Congrats!

All is well, good to go! 🧑‍🍳

proc_dash/layout.py Show resolved Hide resolved
proc_dash/plotting.py Outdated Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
tests/test_app.py Outdated Show resolved Hide resolved
tests/test_app.py Show resolved Hide resolved
tests/test_app.py Show resolved Hide resolved
@alyssadai alyssadai merged commit bf2fb68 into main Oct 18, 2023
2 checks passed
@alyssadai alyssadai deleted the iss-88/add-pheno-histograms branch October 18, 2023 20:08
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 this pull request may close these issues.

Implement plotting of histogram for a user-selected column
3 participants