Do not delete quality and template metrics on recompute #3292
Conversation
|
On a slightly more global discussion point this/or a similar strategy would be needed for template metrics as well right? So that if a new metric is implemented someone can just add that to their old template metrics rather than have to re-calculate. |
|
@samuelgarcia Does this look ok? I'll do a similar thing to |
|
much like adding |
I like to live dangerously |
| @@ -3,6 +3,36 @@ | |||
| import pytest | |||
|
|
|||
|
|
|||
| def test_compute_new_template_metrics(small_sorting_analyzer): | |||
|
|
||
| template_metric_extension = small_sorting_analyzer.get_extension("template_metrics") | ||
|
|
||
| assert "half_width" not in list(template_metric_extension.get_data().keys()) |
There was a problem hiding this comment.
same here might as well check against full default list if it was indeed all computed in the first call
|
Hey @chrishalcrow this is looking nice! I am not very familiar with the |
Thanks - I had totally messed up the template_metrics. The quality_metrics are a bit simpler, I hope! |
Hello, hopefully One big bug I missed: you can't pass any directly parameters to So you'd like set_params to just deal with parameters and run to just deal with computing, but you can't tell run which metrics to calculate. So you've got to sneak them in using a property of the extension class.
I also wasn't meaning to recompute everything but it was tricky to see because the end result was the same and these are pretty fast to compute. Not sure how you could test this. Don't look at |
|
Hello, response to @samuelgarcia done and One bit of a pain: some metrics are named one thing, like @JoeZiminski this is ready for an inspection! I think it's a lot clearer now! |
@chrishalcrow good point! I think it would be great to keep an internal dict of quality metric name to metric table column names! |
| @@ -53,3 +53,29 @@ | |||
| "drift": compute_drift_metrics, | |||
| "sd_ratio": compute_sd_ratio, | |||
| } | |||
|
|
|||
| # a dict converting the name of the metric for computation to the output of that computation | |||
There was a problem hiding this comment.
@alejoe91 You are in luck my friend (will figure out how to test this soon...)
| existing_params = tm_extension.params["metrics_kwargs"] | ||
| # checks that existing metrics were calculated using the same params | ||
| if existing_params != metrics_kwargs_: | ||
| warnings.warn( |
There was a problem hiding this comment.
I think if @h-mayorquin has time he could comment on the value of this particular warning. Not to slow down this PR if it is ready otherwise.
There was a problem hiding this comment.
In defence of the warning:
Suppose a user is going through a large set of sorting_analyzers that they computed over a long time span. They want to compute a new template_metric that they’ve heard about. They know it takes a long time to recompute everything, so they just specify the new metric to compute. Sometimes all the other metrics are deleted! Sometimes they are not!! What is going on?? This warning would have told them. Plus it’s quite rare to get the warning. The default behaviour (compute everything) will never trigger it.
Another option: if the kwargs are different, there is an error telling the user to either use the same kwargs are before or to include all the previous metrics in their compute (quite a complicated error)
There was a problem hiding this comment.
I am not familiar enough with the sorting analyzer workflow to have a useful opinion on this one.
I think it would be better if you had the parameters (the new and the old) in the warning string to give the user a better context.
Something to think about as well is whether this will be called in a context where the warning will make no sense at all for the user. Say, somebody wraps a sorting analyzer and keeps recomputing then the user gets a warning that they don't know anything about. For those cases, it is good to have an option to turn off a sure warning.
But I don't really know how this is used to judge if this last concern is valid or not.
There was a problem hiding this comment.
Ooh yes, I like the idea of printing the old and new parameters!
(I'll add that tomorrow morning!)
| delete_existing_metrics = self.params["delete_existing_metrics"] | ||
| metrics_to_compute = self.params["metrics_to_compute"] |
There was a problem hiding this comment.
@chrishalcrow one last comment. IMO the metrics_to_compute is not really a param, but rather a run-time thing. Would it make sense to move the logic here? As a user, seeing that metrics_to_compute parameter in the json file would make me feel weird!
There was a problem hiding this comment.
Hello! You are logically correct, but run cannot receive any additional arguments, and params is the only way we can pass on the information (also for some technical reason about extensions being reloaded? @samuelgarcia )
So we can either repeat this code in run, or pass this information on using params
There was a problem hiding this comment.
Yes, what I'm saying is since the metrics to compute matter when it's time to run, why not just move the code here to avoid having to save the metrics_to_compute parameter? We have access here to metric names and delete existing, so we have everything we need!
There was a problem hiding this comment.
I had a go but it’s not so simple. We need to set the metric_names in set_params, since this is used for the metadata and in other places (like in get_data). So the params dict needs to know all the metric which are gonna be contained in the extension: everything that has been and will be computed.
So we have access to what’s already been computed, in the previous extensions params, and the complete list of metrics in self.params["metric_names"]. Using this, we can’t distinguish between:
- a metric which the user included in
metric_namesand was already computed. We want to recompute this because the user has explicitly specified it. - a metric which the user has not specified in
metric_namesand was already computed. We don’t want to recompute this because it was already computed.
So we can either
- Change the behavior so that we don’t re-compute metrics that the user specifies and exists already (I think this is bad: the user might be re-comuting because a metric was updated/fixed, and this wouldn’t overwrite the old result, even when explicitly included by the user)
- Give
metrics_to_computea better name in params. Something like a catchier version of “metrics_computed_on_most_recent_run`? - Keep it as is.
- Be cleverer than I've been so far???
There was a problem hiding this comment.
I see! Makes total sense. Let's leave it as is then and thanks for explaining.
From my comment I didn't consider the option that the user is passing a metric name that is already computed in order to recompute...
chrishalcrow
changed the title
| "nn_noise_overlap": ["nn_noise_overlap"], | ||
| "silhouette": ["silhouette"], | ||
| "silhouette_full": ["silhouette_full"], | ||
| } |
There was a problem hiding this comment.
We should maybe have a class approch one day...
Co-authored-by: Garcia Samuel <[email protected]>
|
Merci Chris! |
Possible solution to #3256 and some problems in #3251
This PR alters the way quality_metrics are computed, so that ones which are computed are appended to the extension data, rather than replacing it. This is done by using a new initialisation of the quality_metrics extension.
This might be controversial. Some points in its favour:
Happy to see other solutions to this problem: @samuelgarcia suggested a new method called
append(or something similar) but I couldn't figure out nice syntax for this.