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 Metrics filtering for Conditional Grouping #86

Conversation

jescalada
Copy link
Collaborator

This PR adds Metrics to the list of fields we can filter conditionally for splitting charts. Because each run may have a different value for each metric, only the last value is considered when evaluating (as per Marin's suggestion).

Note that the conditional filtering is only available on the Metrics app at the moment.

Changelog

  • Add metrics to the list of selectable fields (conditionalGroupingOptions)
  • Add metric filtering logic
    • Metrics are filtered based on the last logged value
  • Add tooltip explaining metric filtering behaviour
  • Shorten feature description on popup so it looks better on Google Chrome

Copy link
Collaborator

@fabiovincenzi fabiovincenzi left a comment

Choose a reason for hiding this comment

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

Looks really good!
I tryied a bit and I wasn't able to group by metric context, maybe I'm not using the right syntax.
Screenshot 2024-06-12 at 16 31 55

@jescalada
Copy link
Collaborator Author

Good catch Fabio! I'll take a look at it

@jescalada
Copy link
Collaborator Author

I managed to fix it by adding the context to the attributes to evaluate. I also double checked that there aren't any important attributes that we might want to put in there in the future. The issue with the duplicated entries was fixed by checking for unique values.

There is currently an issue with the metrics displaying as metrics.undefined on Firefox. That being said, I think it might be related to improper state being stored so I'd like to merge everything in first and test with a clean slate!

Copy link
Contributor

@suprjinx suprjinx 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 -- is there a ticket/issue for the todo items?

@jescalada
Copy link
Collaborator Author

Just added one! G-Research/fasttrackml#1295

@jescalada jescalada merged commit 94bce15 into G-Research:release/v3.17.5 Jun 17, 2024
3 checks passed
vinayan3 pushed a commit to vinayan3/fasttrackml-ui-aim that referenced this pull request Aug 28, 2024
* Add conditionalGroupingOptions prop

* Add metrics support to ChartPopover (Metrics app)

* Add condtionalGroupingOptions state

* Add TODO for Params and Scatters

* Fix context conditional grouping and repeated metrics

---------

Co-authored-by: fabiovincenzi <[email protected]>
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.

Metrics are not listed in the conditional grouping dropdown options
3 participants