Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

Dashboard version 1.0, datatable and murakami plot #61

Merged
merged 101 commits into from
Jul 19, 2023
Merged

Conversation

lauraporta
Copy link
Member

@lauraporta lauraporta commented Jun 13, 2023

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed? / What does this PR do?
Here there is a finalised version of the dashboard structure which includes:

  • general scaffold of the dashboard, divided in pages
  • use of storage (dcc.Store)
  • common stylesheet (styles.css)

Also, there I added the simplest plot as an example (the Murakami plot). You can find placeholders for the two other pages, which are empty. I will add their plots in another PR.

How to test locally

First analyse some data by runnign python3 demo_cli.py and then run the dashboard via python3 demo_dash.py.

References

Dash plotly docs for multy-page app

How has this PR been tested?

Testes manually...
I couldn't made any unit test 😭 see #68

Is this a breaking change?

No

Does this PR require an update to the documentation?

Docstrings added

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@lauraporta lauraporta marked this pull request as ready for review July 4, 2023 18:36
@lauraporta lauraporta linked an issue Jul 5, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #61 (d1f13dd) into main (32ea410) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #61   +/-   ##
=======================================
  Coverage   77.10%   77.10%           
=======================================
  Files          14       14           
  Lines         747      747           
=======================================
  Hits          576      576           
  Misses        171      171           
Impacted Files Coverage Δ
rsp_vision/analysis/spatial_freq_temporal_freq.py 95.55% <ø> (ø)
rsp_vision/objects/SWC_Blueprint.py 95.23% <ø> (ø)

@lauraporta lauraporta changed the title Dashboard version 2, datatable and murakami plot Dashboard version 1.0, datatable and murakami plot Jul 5, 2023
Copy link
Member

@JoeZiminski JoeZiminski left a comment

Choose a reason for hiding this comment

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

Hey Laura this looks great, very intuitive to use and nice plots! In terms of setting up the repo for testing, everything is well documented and worked nicely. The dashboard looks really cool, the design is intuitive / easy to use and I like the eye at the top. I am not very familiar with Dash-Plotly so cannot comment deeply on those code sections, but they looked clean and simple to follow / well documented to me. Below I've left some comments as I used the GUI, I put anything that came to mind so please feel free to ignore!

  • I got the warning WARNING:root:ROI 8 and direction <built-in function dir> failed to fit.Skipping.... I think dir should be dir() on line 637 of analysis.py.
  • I initially expected the plot to open automatically but I had to click the link. Can / should it open automatically?
  • The button Toggle Columns did not do anything I could see.
  • the main entry function app.py does not take any arguments, but it could possibly be a function that has a long docstring also explaining how to setup configs / what it does, as I guess as an entry point it will be well-viewed by users.
  • clicking the filtering data column days_of_the_experiment resulted in an error.
  • The "filter data" text is always shown for the first column but only on hover for the second column, if unintended this could be changed for consistency.
  • It is already very clear, but Load could be changed to Load Selected Row just to be super explicit.
  • For the column headers on the table, is it possible to use full words rather than variable names? e.g. Folder name rather than folder_name.
  • The plot is very nice! Some minor things 1) on my screen there is quite a lot of white-space to it's right hand side. Can it be made a bit bigger or more centred? These things are often not simple, it is good as-is but just wondering. 2) By default, the plot is sized to all datapoints even if they are not shown (i.e. hidden unresponsive units). Can the plot be re-sized depending on the displayed ROIs? Currently, if red-dots only are displayed they huddle in the bottom right corner because the plot is sized to both red and black dots.
  • The axes are logarithmic scale, is it worth mentioning this on the labels and/or will it ever be required to provide a button to switch between log and standard scale? (or maybe these units are standardly in log and it is not worth mentioning).
  • It's very nice to hover on the plot and see the ROI. For the responsive units on the standard test dataset, I see two ROI 8 and one ROI 9, is this okay?
  • Now might be a nice time to write user-docs for the plot in terms of what exactly they show, what is a Murakami plot etc. (might be easier when things are more fresh in memory).
  • Currently the highlighting on the left-hand panel of the Murakami page is ligher for selected, darker for unselected. Does it make sense to have darker for selected and lighter for unselected?
  • That's great to export the plot as PNG, is it also possible in dash-plotly to export as svg?
  • The tab for the website (i.e. in a internet browser) is currently 'Data Table' maybe this can be changed to 'RSP Vision'.
  • Maybe the text on the Murakami plot page can be new-lined as below.
Responsive ROIs are shown in red, 
Non-responsive ROIs are shown in black.

-Out of interest, how does the dashboard fit into the overall pipeline? I guess it goes analysis - explore, check, produe figures using dashboard, are there any additinal analysis stages? Do all subjects need to be pooled at some point?

Overall excellent work!

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
rsp_vision/dashboard/pages/data_table.py Outdated Show resolved Hide resolved
rsp_vision/dashboard/pages/data_table.py Outdated Show resolved Hide resolved
rsp_vision/dashboard/pages/polar_plots.py Outdated Show resolved Hide resolved
rsp_vision/dashboard/pages/polar_plots.py Outdated Show resolved Hide resolved
rsp_vision/dashboard/pages/sf_tf_facet_plot.py Outdated Show resolved Hide resolved
@lauraporta
Copy link
Member Author

Hi Joe, thanks for the feedback! You spotted many bugs 😱
Here I reply to some of your points for which I didn't made an issue or corrected the code.

Issues created:

#71
#72

app.py docstring

Yes I agree ther should be more descriotion of what it does. Its functionalify is now under development in another branch because in which I am changing the methods into something that it is easily callable with a dash script and can run all the analysis in parallel in the hpc. Once I decide how I want that bit to operate I will add docstrings

Data Table

filtering errors

It is weird, sometimes the filtering works and other times does not. It works if I refresh the page. I can make an issue on it.

changing column names

This would be easy to do if I was displaying all the columns in a df but it becomes a particular edge case given that I am also hiding some of them.

Murakami plot

Resizing

I explicitly no not want to resize the plot because I think it is important to understand where the ROIs peaks are located in the range of all sf/tf. It is ok to see them all in the corner. The idea is that people will switch between different datasets and for comparison it is useful to have the axis stable in the viz.

Axes

I had the switch between log and linear scale in the previous version but I didn't fully like it. I am skipping it for now unless I will be asked by the user to add it.

User docs

Definitely needed, but coming later

Buttons

My idea was that what is selected is the current page and therefore it does not make sense to click it. Therefore I marked the relative button as disabled which makes it appear grey.

@lauraporta lauraporta merged commit 1a91399 into main Jul 19, 2023
@lauraporta lauraporta deleted the dashboard-01 branch July 21, 2023 12:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Analysis behind murakami plot Murakami plot Dashboard design
2 participants