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

Add ability to generate and view reports for any run. #716

Merged
merged 11 commits into from Jan 14, 2020

Conversation

rwblair
Copy link
Collaborator

@rwblair rwblair commented Jan 9, 2020

Fixes #533 #686

Currently no warning on number of runs selected. @adelavega any idea at what number we might want to warn at?

This also assumes that the api returns run plots in the same order that front end asks for them.

@codecov-io
Copy link

codecov-io commented Jan 9, 2020

Codecov Report

Merging #716 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #716   +/-   ##
=======================================
  Coverage   85.66%   85.66%           
=======================================
  Files          66       66           
  Lines        2936     2936           
=======================================
  Hits         2515     2515           
  Misses        421      421

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fffcf78...21a1687. Read the comment docs.

@adelavega adelavega self-requested a review January 9, 2020 23:01
Copy link
Collaborator

@adelavega adelavega left a comment

Choose a reason for hiding this comment

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

Nice, I actually think it looks pretty good!

  • One issue is when I closed tabs using the browser, leftover tabs remained:

Screenshot from 2020-01-09 16-54-01

  • It would be nice if it took a bit less space at the top, especially for smaller screens... I wonder if we could stick it somewhere below, or hide behind a pop up / modal?
    Would it be possible to this element into into a modal that pops up if you click a tab with a + icon on it?

  • Maybe we should spit a warning out / confirmation dialog if more than 4 reports are requested?

There's a few things we could sort to make things look nicer.
Let's always sort by Subject, Session (if any, currently there are none), then Run.

  • Let's sort by this order in the string formatted representation of the runs
  • Sort the order of the runs in the drop down
  • Finally, let's request the first run in this sorted order. E.g. Subject 1, run 1. Rather that what were doing now which is requesting the first run, with the lowest ID. Which is sort of a random order.

Otherwise looks good, and performance looks like not a problem. I have it set on the server right now so maybe @rbroc can also give her feedback

@adelavega
Copy link
Collaborator

Here's a fun state related bug I found. When I go to an already compiled analysis, it lands on the Review tab. This tab is blank when you first land on it.

Try it here: https://neuroscout.org/builder/MNW3V

But if I go to "Status" then back to "Review" it loads properly...

@rbroc
Copy link
Collaborator

rbroc commented Jan 10, 2020

Format looks awesome, just a couple of points:

  • You can type stuff in the search box but it does not seem to be searching on the basis of the same type of subject IDs used in the dropdown list (if you try typing "s" in the searchbox, it would not match any IDs, despite all IDs in the dropdown list being "subject-something" - e.g. in AxkxB). Could we switch to searching based on same ID type used in the dropdown?

  • testing this with analysis AxkxB (compiled and public). The report page first loads "subject - 15". If I add one more subject to that, it displays two tabs but in both tabs there's the same design matrix (note that I am using fmriprep motion parameters as regressors, so they should be different). Maybe I am missing something, but see if you can replicate that.

  • Agree on the ordering of subjects in the dropdown, it's a minor thing but it'd make it nicer.

  • Being able to inspect different runs would be awesome!

@rwblair
Copy link
Collaborator Author

rwblair commented Jan 13, 2020

Latest push should address

  • hanging tab on run removal from select bar
  • run ordering in select list
  • allow filtering in select
  • Only runids selected in overview should be present in select list.
  • Review component not displaying on initial load of passed analysis

@adelavega
Copy link
Collaborator

adelavega commented Jan 13, 2020

Awesome, just tested your latest commit and I think almost everything is addressed (including showing all the runs, and filtering based on the runs selected in the first tab).

I think the only remaining concerns are:

  • Move into a modal? I think we can probably wait on this, and do it in another PR if it keeps bugging us.
  • Warn if more than 3 runs are selected at once. I think this might be more impotant. Maybe once a 4th is added, a pop up warning says something? Or it could be on the "Generate Report" click that it makes you confirm yes/no if you really want to do this.
  • Have the formatted string go in this order: "Subject" "Session" "Run". I'm still seeing "Run" go first.

Otherwise, LGTM!

@adelavega
Copy link
Collaborator

Hey @rwblair can we also take care of #697 here? It would simply be a link to design_matrix which is given by the report. I think a button below the design matrix plot would suffice.

@rwblair
Copy link
Collaborator Author

rwblair commented Jan 14, 2020

Added link to download design matrix as a png. This png is being done when the design matrix component is loaded. We can push this generation off to when the download link is clicked if this is a performance issue but it will take some work ( think we need to generate png, make anchor tag, add it to dom, use reference to anchor to call its click() function.)

@adelavega
Copy link
Collaborator

Oh, sorry Ross. Miscommunication, I meant a link to download the .tsv design matrix. I think you can already download thos einteractive plots by hovering over the ...

@adelavega
Copy link
Collaborator

The tsv is available as the key design_matrix in the result. The way the link looks would be fine. I also wonder if the last change introduced a bug, because now I'm requesting two runs, and getting back only a single tab. Hopefully reverting these changes will undo that.

@rwblair
Copy link
Collaborator Author

rwblair commented Jan 14, 2020

Ah good I'll revert the changes. Knew about the menu in vega embed, but figured we might want it more explicit? Any way downloading tsv much better.

@adelavega
Copy link
Collaborator

On a very wide screen, like my external monitor, the link gets split up:

image

@adelavega
Copy link
Collaborator

Otherwise, LGTM!

@rwblair
Copy link
Collaborator Author

rwblair commented Jan 14, 2020

Try that latest fix commit, I tried zooming out on my low res monitor and couldn't get it to float to the side.

@adelavega
Copy link
Collaborator

That fixed it, merging!

@adelavega adelavega merged commit a0295ba into master Jan 14, 2020
@adelavega adelavega deleted the 680_reports_not_updating branch January 14, 2020 21:07
@adelavega adelavega mentioned this pull request Jan 14, 2020
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.

Allow user to select subject/run to display design matrix for
4 participants