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

[DOC] Reporting documentation #465

Merged
merged 84 commits into from
Oct 19, 2020
Merged

[DOC] Reporting documentation #465

merged 84 commits into from
Oct 19, 2020

Conversation

javiergcas
Copy link
Collaborator

@javiergcas javiergcas commented Nov 14, 2019

Closes None.

Changes proposed in this pull request:

  • First draft of documentation for reports
  • Figures described
  • Interaction description missing

@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #465   +/-   ##
=======================================
  Coverage   93.53%   93.53%           
=======================================
  Files          26       26           
  Lines        1965     1965           
=======================================
  Hits         1838     1838           
  Misses        127      127           

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 5f64cbc...252f7c3. Read the comment docs.

@emdupre emdupre changed the title Reports doc [DOC] Reporting documentation Nov 14, 2019
Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

Only reviewed docs instead of code since I'm not familiar with the python-JS cross-talk employed with this PR.

docs/reporting.rst Outdated Show resolved Hide resolved
docs/reporting.rst Outdated Show resolved Hide resolved
-------------------------
This view provides detailed information about an individual
component (selected in the summary view, see below). It includes three different
interactive plots.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So should we just not list the plot as interactive?

requirements.txt Outdated Show resolved Hide resolved
@eurunuela
Copy link
Collaborator

Only reviewed docs instead of code since I'm not familiar with the python-JS cross-talk employed with this PR.

Thank you so much for the review @jbteves !

I'm not sure I understand what you mean by this though: "So should we just not list the plot as interactive?"

I added a link to the OHBM demo to the very top of the reporting page, so people can play around with it. The images in the docs are all static.

@eurunuela eurunuela requested review from jbteves and tsalo October 16, 2020 08:09
Copy link
Member

@emdupre emdupre left a comment

Choose a reason for hiding this comment

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

Thanks again, @eurunuela and @javiergcas ! I think this will be a huge help to users.

I've made a few comments / suggestions to bring the docs in line with the current reporting functionality.

docs/reporting.rst Outdated Show resolved Hide resolved
The image below shows a representative report, which has two sections: a) the summary view,
and b) the individual component view.

.. image:: /_static/rep01_overallview.png
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this reflects the state of the reports any more, unfortunately, since we removed the individual component view. The figure should be updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

1b59407 should take care of this.

I included the individual component view section in the end, as we do show a static view of individual components.

Copy link
Member

Choose a reason for hiding this comment

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

That looks right, thanks !! Should we note that the individual component view will be empty by default, since no component is pre-selected ?


* **Kappa Scree Plot:** This scree plots provides a view of the components ranked by `Kappa`
As in the previous plot, each dot represents a component. The color of the dot inform us
about classification status. In this plot, size is not related to variance explained.
Copy link
Member

Choose a reason for hiding this comment

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

Should we explain what the axes are ?

docs/reporting.rst Outdated Show resolved Hide resolved
docs/reporting.rst Outdated Show resolved Hide resolved
docs/reporting.rst Outdated Show resolved Hide resolved
=========================

As previously mentioned, all plots in the report allow user interactions. The list of permitted
interactions vary by plot, but can be easily infered by the toolbar that accompanies each plot
Copy link
Member

Choose a reason for hiding this comment

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

In the current view, the toolbar is only on the Kappa-Rho plot, so we likely need to rephrase this section accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

1b59407 should take care of this.

@eurunuela eurunuela requested a review from emdupre October 18, 2020 10:52
:align: center
:height: 150px

.. note::
Copy link
Member

Choose a reason for hiding this comment

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

This is what I was thinking of 😸 Maybe let's move it up around L19 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes! That's a great idea. Thank you!

emdupre
emdupre previously approved these changes Oct 18, 2020
Copy link
Member

@emdupre emdupre left a comment

Choose a reason for hiding this comment

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

One small suggestion from me, and I think this looks great. Thanks again !

Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

Found a few typos, but otherwise looks good with the continuous-docs render.

docs/reporting.rst Outdated Show resolved Hide resolved
docs/reporting.rst Outdated Show resolved Hide resolved
docs/reporting.rst Outdated Show resolved Hide resolved
Co-authored-by: Joshua Teves <[email protected]>
eurunuela and others added 2 commits October 19, 2020 16:17
Co-authored-by: Joshua Teves <[email protected]>
Co-authored-by: Joshua Teves <[email protected]>
@eurunuela
Copy link
Collaborator

Found a few typos, but otherwise looks good with the continuous-docs render.

Thank you! I thought I had already corrected those.

@eurunuela eurunuela requested review from jbteves and emdupre October 19, 2020 14:22
Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @eurunuela and @javiergcas !

Copy link
Member

@emdupre emdupre left a comment

Choose a reason for hiding this comment

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

LGTM ! 🚀 Merging now. Thanks again !!

@emdupre emdupre merged commit 0eaa3ba into ME-ICA:master Oct 19, 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.

6 participants