Skip to content

Conversation

@isabelizimm
Copy link
Contributor

@isabelizimm isabelizimm commented Jun 30, 2022

adds

  • plot_metrics + tests
  • compute_metrics + tests

With pinning in separate PR to round out all the monitoring! 🎉

@isabelizimm isabelizimm marked this pull request as ready for review July 1, 2022 21:05
@isabelizimm isabelizimm requested review from juliasilge and machow July 1, 2022 21:17
@machow
Copy link
Contributor

machow commented Jul 5, 2022

I can take a closer look in a little bit, but one quick thought -- can we add some extra tests of the rolling window? Vetiver R has the luxury of leaning on the slider package 😭!

Looking at the code, it might be good to test against these potential bad things:

  • the rolling window accidentally produces the same dataframe for each window
  • the metric doesn't get applied per window, so returns the same result for each window

Another quick thought. a lot of R functions can use the actual variable name in the output. vetiver-python matches this using .__qualname__. But there are often cases where .__qualname__ doesn't exist, like object instances and relatedly functools.partial. I wonder if it'd be useful to provided a output naming escape hatch, like allowing a dictionary of metrics metric functions.

@isabelizimm isabelizimm merged commit 85fef87 into rstudio:main Jul 5, 2022
@isabelizimm isabelizimm deleted the dev-model-monitoring branch July 5, 2022 18:31
@isabelizimm isabelizimm mentioned this pull request Jul 21, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants