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 series visualisations #2598

Merged
merged 210 commits into from
Aug 24, 2021
Merged

Add series visualisations #2598

merged 210 commits into from
Aug 24, 2021

Conversation

tibdhond
Copy link
Contributor

@tibdhond tibdhond commented Apr 8, 2021

This pull request adds a number of visualisations to the course page. All of the visualisations are aimed at showing the status of the exercises in a single series. The visualisations are only visible for course admins.

Graphs

Tries needed to get it right

The first graph shows the distribution of number of submissions for each of the exercises. The average number of tries is shown with a dot and explicitly shown at the right. A mouseover shows the exact values.

image

For now, the x-axis is always limited to 20. User with more submissions are binned in that last bin.

Distribution of the submission statuses

The second graph shows a bar chart for each exercise showing the distribution of the status of all submitted solutions. This can be useful to see if the time or memory limit might be too strict. A tooltip shows the exact values

image

When do students practice

This graph is unfinished. It needs better (dynamic) binning to be useful. It is therefore hidden in the final version of this PR.

image

When did the students get it right

This graph shows the percentage of students that have submitted a correct solution to an exercise over time. It can be useful to monitor the progress of students after a lab session or the amount of work that was spent just before the deadline. A tooltip shows the exact percentages.

image

Next steps

There are a number of follow up issues that would improve this first version.


Closes #1835, closes #1013, closes #984

@chvp chvp added the feature New feature or request label Apr 12, 2021
@tibdhond
Copy link
Contributor Author

tibdhond commented Apr 14, 2021

The visualisations have now been integrated into the course page. However, since there isn't a lot of dummy data available, it's hard to say what should change in terms of styling (of the graphs themselves). This requires some further application to more realistic data.

image

image

@tibdhond
Copy link
Contributor Author

The latest version has been deployed to naos with the following results:

image

image

image

image
The black spots here are caused by unfinished code that was accidentally pushed.

@tibdhond
Copy link
Contributor Author

tibdhond commented Apr 20, 2021

Changes that should still be made:

General

  • proper usage of localisation
  • proper usage of authorization
  • sort exercises in right order
  • hide stats button for non-course admins
  • more padding to the left of graphs / a way to make sure titles aren't cut off
  • some sort of explanation on how to use/interpret the graphs
  • different styling / positioning for the graph tabs?
  • switching between graphs causes the card to grow
  • use cache

Violin

  • Decide on a colour for the graph
  • Reposition x-axis label

Stacked Status

  • Decide on colours for each status
  • tooltip hidden behind graph tab

Timeseries

  • Decide on colours for each status
  • fix time scoping problem
  • only display min and max values on y-axis
  • re-enable interpolation (while making sure the graph doesn't drop below 0)
  • (empty) datapoints for every day + ensure first and last day of scope are shown
  • toggle cumulative button
  • toggle shared y-axis button

@tibdhond
Copy link
Contributor Author

Some issues:

Authorization
Not quite sure how to make use of the authorization.
I assume I'd have to do something along the lines of authorize course or authorize series in statistics_controller.rb and create new policies in series_policy.rb or course_policy.rb?

Exercise order
How do I determine the exercise order? Is it safe to assume they will be in the correct order when fetching them from a series object?

minor problem: typescript error
I'm getting several typescript errors when setting the area (in violin and timeseries). Typescript seems to insist that the input for area must have type [[number, number]], while this shouldn't be necessary. Is there something I can do about this or do I just ignore it?

@tibdhond
Copy link
Contributor Author

tibdhond commented Apr 20, 2021

I tried the green/shades of red colour scheme (using d3.interpolateReds) for timeseries on a local graph with following results:
image

with red shades reversed:
image

I feel like the reds are only barely distinguishable from each other on this large example, so they probably won't be in their reduced size (in the series cards).
The 'wrong' statuses might have to be aggregated if we want to use this kind of colour scheme, or we will have to decide on another one.

@tibdhond
Copy link
Contributor Author

All strings now use proper translations.
Not sure how/whether to translate the stats tab (violin/stacked status/timeseries)

@tibdhond
Copy link
Contributor Author

Retaining the card height causes some slight issues when there's only one exercise in a series:
image

@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Apr 24, 2021
@bmesuere bmesuere temporarily deployed to mestra April 24, 2021 09:34 Inactive
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Apr 24, 2021
@bmesuere
Copy link
Member

Regarding show statistics button:

  • I would add 📈 icon next to the 3 dots to enable them. If graphs are shown, I would switch out the 📈 for a format-list-bulleted icon
  • violin: discuss on Monday if we want a cutoff for outliers. Right now they make the rest less interpretable
  • colors: maybe try if we can use the same color scheme for both plots? The default scheme is a bit ugly, especially in dark mode. The dark2 scheme probably is a better fit (at least in dark mode)
  • stacked: consider grid lines every 20%
  • time series: put exercise labels to the left
  • time series: discuss on Monday: do we take a cutoff date at the deadline? Right now it doesn't look good because too stretched
  • general: discuss on Monday: include reading activities?
  • general: discuss on Monday: I thought we needed to keep the height of the original table to prevent jumps or the lazy loading of series from breaking, but this is not the case so we can reevaluate this.

@tibdhond
Copy link
Contributor Author

tibdhond commented Apr 26, 2021

Some more problems that need fixing:

Timeseries

  • Tooltips don't seem to work
  • sometimes freezes the page (I assume it gets stuck in a loop)

Violin

  • x-axis label overlaps with axis
  • mouse cursor can overlap with graph cursor when graph is small (making it hard to see)
  • fallback when there is no data

Stacked status

  • fallback when there is no data

@tibdhond
Copy link
Contributor Author

tibdhond commented Apr 26, 2021

  • move tab buttons to the right
  • don't reload graph if already selected
  • violin lighter variant of primary colour
  • violin move x-axis label to the left
  • violin bigger tooltip label #users # submissions
  • violin show cursor over all exercises at once
  • stacked status: horizontal legend
  • stacked status: gridlines labels
  • stacked status: expand tooltip with amounts
  • stacked status: total submissions to the right
  • stacked status: move tooltip closer to mouse cursor
  • timeseries: insert empty datapoints for every day
  • cumulative timeseries: only one graph, only first correct, stack exercises

@tibdhond
Copy link
Contributor Author

Violin tooltips have been changed to show tooltips for all exercises at once and the colour has been changed to (a shade of) the primary colour.

image

@tibdhond
Copy link
Contributor Author

Stacked status has undergone some changes:

image

@tibdhond
Copy link
Contributor Author

tibdhond commented May 1, 2021

The cumulative timeseries has been implemented. I went for a line graph instead of an area graph since this format felt like it would be better interpretable. I still have to figure out how to translate the date strings however.

image

@tibdhond
Copy link
Contributor Author

tibdhond commented May 1, 2021

The regular timeseries also had some updates and bugfixes. I feel like the way tooltips are displayed right now could be improved, but can't think of a better way at the moment.

image

@tibdhond
Copy link
Contributor Author

tibdhond commented May 2, 2021

TODOS

  • authorization + hide button
  • translation of graph names? (in the toggle tab)
  • do something about series with no deadline
  • graph information button + tooltip
  • violin: tooltip design (not too happy with how it currently looks)
  • violin: remove bold aspect of labels
  • violin: change tooltip label colour with lightmode/darkmode
  • stacked status: break two tooltips into two lines
  • stacked status: switch position of relative and absolute values
  • timeseries: replace with linear heatmap
  • ctimeseries: translate date strings
  • ctimeseries: fix x-axis ticks
  • ctimeseries: change y-axis to percentage of total subscribed students

@tibdhond
Copy link
Contributor Author

tibdhond commented May 4, 2021

The violin tooltip has received another redesign and now has the following look:

image

@tibdhond
Copy link
Contributor Author

tibdhond commented May 6, 2021

The cumulative timeseries has been changed to display percentages instead of absolute numbers. The tooltips now display both the absolute and relative amounts. This might cause things to get a bit crowded however.

image

@tibdhond
Copy link
Contributor Author

tibdhond commented May 6, 2021

The violin plot icon from the noun project didn't show up for some reason (+ has credentials in free version) so I went with the closest thing I could find.

image

@tibdhond
Copy link
Contributor Author

tibdhond commented May 6, 2021

Authorization has been implemented, but is turned off. This will need to be turned back on before deploying.

@chvp chvp temporarily deployed to naos May 9, 2021 14:53 Inactive
@lgtm-com
Copy link

lgtm-com bot commented May 9, 2021

This pull request introduces 5 alerts when merging d011f84 into 001147f - view on LGTM.com

new alerts:

  • 5 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 9, 2021

This pull request introduces 5 alerts when merging 9ac7488 into 001147f - view on LGTM.com

new alerts:

  • 5 for Unused variable, import, function or class

@tibdhond
Copy link
Contributor Author

tibdhond commented May 10, 2021

TODOS

  • info icon instead of exclamation mark
  • rotate icons
  • try to remove course admins from data
  • use same tooltip label positioning for violin and ctimeseries
  • look into colouring icons
  • tests
  • violin: metrics on the right
  • violin: set letter stroke to background colour
  • stacked status: label on the right styling
  • stacked status: move legend up
  • ctimeseries: use subscribed instead of enrolled
  • ctimeseries: use I18n for translating dates
  • ctimeseries: setup binning by hour
  • timeseries: dynamic rect width

@lgtm-com
Copy link

lgtm-com bot commented May 13, 2021

This pull request introduces 5 alerts when merging dbfb355 into 55ef170 - view on LGTM.com

new alerts:

  • 5 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 13, 2021

This pull request introduces 5 alerts when merging f6e9f59 into 55ef170 - view on LGTM.com

new alerts:

  • 5 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 13, 2021

This pull request introduces 5 alerts when merging c40fb20 into 55ef170 - view on LGTM.com

new alerts:

  • 5 for Unused variable, import, function or class

@chvp chvp force-pushed the exercise-statistics-visualisation branch from 52603bf to 3358cbe Compare August 23, 2021 14:21
@chvp chvp temporarily deployed to naos August 23, 2021 14:23 Inactive
@bmesuere bmesuere requested review from chvp and niknetniko August 23, 2021 14:59
config/routes.rb Outdated
end

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@bmesuere bmesuere marked this pull request as ready for review August 23, 2021 16:30
@bmesuere bmesuere requested a review from a team as a code owner August 23, 2021 16:30
app/assets/stylesheets/components.css.scss Outdated Show resolved Hide resolved
app/controllers/statistics_controller.rb Outdated Show resolved Hide resolved
app/controllers/statistics_controller.rb Outdated Show resolved Hide resolved
app/controllers/statistics_controller.rb Outdated Show resolved Hide resolved
app/models/submission.rb Outdated Show resolved Hide resolved
app/models/submission.rb Show resolved Hide resolved
app/models/submission.rb Outdated Show resolved Hide resolved
app/models/submission.rb Outdated Show resolved Hide resolved
@chvp chvp force-pushed the exercise-statistics-visualisation branch from 1689256 to b617d30 Compare August 24, 2021 12:19
@chvp chvp requested a review from bmesuere August 24, 2021 12:19
@chvp chvp force-pushed the exercise-statistics-visualisation branch 2 times, most recently from aa586c1 to ce0cca6 Compare August 24, 2021 12:32
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

  • When there is no data, the last graph is a bit weird:
    image
  • I would maybe add the short description as a tooltip to the buttons as well

app/assets/javascripts/visualisations/violin.ts Outdated Show resolved Hide resolved
app/assets/javascripts/visualisations/series_graph.ts Outdated Show resolved Hide resolved
end

def series_visualisation(visualisation)
series = Series.find(params[:series_id]) if params.key?(:series_id)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
series = Series.find(params[:series_id]) if params.key?(:series_id)
series = Series.find(params[:series_id])

By removing the if, we'll get 404 on a missing param, instead of a 500. (Technically it should probably be a 422, since the resource is found, but a param is missing, but I don't think we do that anywhere for GET requests)

config/locales/js/en.yml Outdated Show resolved Hide resolved
@bmesuere
Copy link
Member

I added a title attribute to the toggle buttons containing the same string as the title shown above the graph. I didn't use bootstrap tooltips because these use the data-bs-toggle attribute which is already used on those buttons for the tabs.

@bmesuere bmesuere merged commit 77f5aca into develop Aug 24, 2021
@bmesuere bmesuere deleted the exercise-statistics-visualisation branch August 24, 2021 14:13
@bmesuere bmesuere changed the title Exercise statistics visualisation Add series visualisations Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exercise statistics Show exercise progress over time Submission event drops/violin plot
6 participants