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

Analysis view: box & whiskers plot #1355

Merged
merged 18 commits into from
Aug 22, 2023
Merged

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Jun 27, 2023

Supersedes #1290

I've extracted out the relevant diff from #1290 and tidied it up / built upon it

image

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • No docs needed

@MetRonnie MetRonnie added this to the 2.0.0 milestone Jun 27, 2023
@MetRonnie MetRonnie mentioned this pull request Jun 27, 2023
8 tasks
@MetRonnie MetRonnie force-pushed the analysis branch 7 times, most recently from 93128ae to 4597e49 Compare June 30, 2023 15:02
@MetRonnie

This comment was marked as resolved.

@MetRonnie MetRonnie force-pushed the analysis branch 2 times, most recently from 2b4108c to ec157f4 Compare July 20, 2023 09:06
@MetRonnie MetRonnie marked this pull request as ready for review August 7, 2023 12:57
@MetRonnie MetRonnie self-assigned this Aug 7, 2023
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looks good, works well, seems robust.

A few small points:

It would be good to give units with these times (i.e. seconds), worth bearing in mind that we might want to customise the display of these times in the future (e.g. 45s, 10m, 1:20, PT4H31M, etc):

Screenshot from 2023-08-09 17-19-39

I managed to get a null Q1 time where it didn't make sense:

Screenshot from 2023-08-09 17-20-44

It's not completely clear what the chart is displaying, e.g. what do the blue/green numbers mean, especially when there is little spread in the times so far:

Screenshot from 2023-08-09 17-21-57

@ChrisPaulBennett
Copy link
Contributor

The null entries are caused by a bug in the SQL in the UI Server.
A pull request has been created for this (cylc/cylc-uiserver#483).
Currently investigating the other issues

MetRonnie and others added 2 commits August 21, 2023 16:37
Co-authored-by: ChrisPaulBennett <christopher.bennett@metoffice.gov.uk>
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

👍

src/views/Analysis.vue Outdated Show resolved Hide resolved
@oliver-sanders oliver-sanders merged commit b7eb657 into cylc:master Aug 22, 2023
6 checks passed
@MetRonnie MetRonnie deleted the analysis branch August 22, 2023 14:27
@MetRonnie MetRonnie added the data workflows team Work pertaining to the analysis/gantt/etc views label Apr 9, 2024
@MetRonnie MetRonnie mentioned this pull request Apr 9, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data workflows team Work pertaining to the analysis/gantt/etc views
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants