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.

Sign up for GitHub

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

Jump to bottom

UI: New rule metrics layout #861

Merged
merged 24 commits into from Jan 4, 2022
Merged

UI: New rule metrics layout #861

merged 24 commits into from Jan 4, 2022

Conversation

leiyre
Copy link
Member

@leiyre leiyre commented Dec 30, 2021 •
edited by frascuchon

Closes #860
Closes #867

@leiyre leiyre changed the title UI/new rule metrics UI: New rule metrics layout Dec 30, 2021
@dvsrepo
Copy link
Member

dvsrepo commented Dec 30, 2021 •
edited

Looks good!

Some comments:

  • There's a resize of the rule labels box when saving a rule (it becomes smaller). We should keep the size of the box as stable as possible. See gif below and next comment.

  • If possible it would be great to reduce the "refresh" effect on the rule labels and rule metrics boxes. Ideally those should not be refreshed completely (only the numbers, the labels, etc.) and should keep a stable, fixed size. The record list below of course needs to be refreshed. I don't know if its a lot of work, if it's not too much I would tackle this for the release. See gif below.

  • In the label definition box, is the number of records available to show it before selecting a label ( Records 0/11.100
    )? If can be known before selecting a label we should show it, otherwise is fine.

review_rules

@Amelie-V
Copy link
Member

Amelie-V commented Dec 30, 2021 •
edited by leiyre

Define rule

  • Blue block: Global metrics should be bolder (check UI)
  • Blue blocks: In both view (define rules and rules list) add a bit more margin (30 instead of 20)
  • Define rule area: query name should be larger (like in the last version, (same size as "New Query")
  • Save rule button should be fixed at the bottom (just like the info when rules is already saved)
  • The goal is to fit 3 lines of labels and then show : "+ X labels" (just in case, i cant test it)
  • Spaces between search box and the 2 blocks should be 10px (just like between the two blocks)

@dvsrepo
Copy link
Member

dvsrepo commented Dec 31, 2021 •
edited by leiyre

@leiyre I've reviewed all raised points and it's looking good!!

Testing I have found these two issues:

  • Avg. precision and total incorrect/correct suddenly become 0 both in rule definition and rule management views (see gif)
  • When a rule is saved, you are able to change the label but clicking on save label has no effect.

review_rules2

@leiyre
Copy link
Member Author

leiyre commented Jan 3, 2022 •
edited

@Amelie-V

  • The goal is to fit 3 lines of labels and then show : "+ X labels" (just in case, i cant test it)

The width is dynamic, it's better to define a fixed number of elements. By default is a max of 10

  • Spaces between search box and the 2 blocks should be 10px (just like between the two blocks)

The spacing is common to all views, the bottom margin of the seacrhbox is 14px, if we change it to 10px it will affect all views. it's ok?

@leiyre leiyre closed this Jan 3, 2022
@leiyre leiyre reopened this Jan 3, 2022
@frascuchon
Copy link
Member

frascuchon commented Jan 3, 2022 •
edited

Comment from my part:

The message "The query is already saved as a rule" when a rule is already stored it could be confusing if we only change the label:

Screenshot 2022-01-03 at 11 40 05

Screenshot 2022-01-03 at 11 40 12

In both cases the query remains the same. As suggestion (but sure I'm not an expert) keeping the save rule button enabled/disabled depending on the rule was already created. Or maybe change the action name (Create Rule/ Save rule)

@Amelie-V ?

@dvsrepo
Copy link
Member

dvsrepo commented Jan 3, 2022

I agree. The message is confusing and the interaction too.

In my opinion:

  1. Improve the message to: This query with this label are already saved as rule.
  2. Showing disabled button is a good idea, but I would keep the message otherwise it will still be confusing.
  3. The message after saving the rule shouldnt be the message above, but: The rule has been saved.

@frascuchon
Copy link
Member

  1. The message after saving the rule shouldnt be the message above, but: The rule has been saved.

We have already a global mechanism to notify completed actions. I prefer show this message as a toast notification.

@frascuchon
Copy link
Member

It could be nice to include some color schema to highlight the best rule metrics. The greater value for precision, the better, or the lower value for incorrect records, the better too. This could help to users to analyze rule

@frascuchon
Copy link
Member

frascuchon commented Jan 3, 2022 •
edited

Affected records count (Records: ...) should be shown only if we have label info. Otherwise, it's quite confusing:

Screenshot 2022-01-03 at 14 47 25

The same could be applied to coverage and annotated coverage (Giving 0% of coverage while rule query returns results is confusing)

@leiyre
Copy link
Member Author

leiyre commented Jan 4, 2022

fix #867

@frascuchon frascuchon self-requested a review January 4, 2022 10:41
frascuchon
frascuchon approved these changes Jan 4, 2022
View reviewed changes
Copy link
Member

@frascuchon frascuchon left a comment

Choose a reason for hiding this comment

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

Closing this PR and all pending tasks must be transcribed as separate issues

@frascuchon frascuchon merged commit d6a32dc into master Jan 4, 2022
@frascuchon frascuchon deleted the ui/new_rule_metrics branch January 4, 2022 10:42
dvsrepo added a commit that referenced this pull request Jan 11, 2022
@dvsrepo
Merge branch 'master' of https://github.com/recognai/rubrix
2adcf12 
* 'master' of https://github.com/recognai/rubrix:
  Docs: Updates Flair zeroshot tutorial (#887)
  removing wrong video (#885)
  Update readme (#883)
  fix(UI) Metrics value by default if no metric (#875)
  feat(metrics): add token level metrics for token classification from client (#849)
  UI: New rule metrics layout (#861)
  chore: expose load_rules from base module (#866)
frascuchon pushed a commit that referenced this pull request Jan 11, 2022
@leiyre @frascuchon
UI: New rule metrics layout (#861)
1fa8aa4 
* remove old rule metrics

* remove old overall metrics

* new metrics component

* restructure metrics

* styles

* button position

* metrics min height

* metrics loading placeholder

* fix metric item height

* show loading only in records area when refresh

* fix padding and font-size

* fix update total metrics

* fix lint

* show all non-reactive buttons

* fix styles

* rules props

* text style

* change copy

* labels as array

* prevent empty rules

* fix empty search for data filter

* nw rule empty query component

* refresh only general metrics when updating the rule

* overall metrics transition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frascuchon frascuchon frascuchon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Rules] "Delete rule" and "update summary" actions should be consistent [rules] New metrics layout
4 participants
@leiyre @dvsrepo @Amelie-V @frascuchon