Kendall's Tau metric, based loosely on scipy. #2169
Conversation
|
You are owner of some files modified in this pull request. |
|
LGTM! Thanks again. Some test are failing but they are on our end and not yours. Once we fix them, I think we can merge this. |
|
/cc @brianwa84 @jvdillon Are you interested in this for the Tensorflow Probability repo? It could fit in the stats sub package: https://www.tensorflow.org/probability/api_docs/python/tfp/stats |
|
Hello! I'd love to see this in TFP! Id also love to see the top-k loss, as described here: https://arxiv.org/pdf/1206.5280.pdf |
| @@ -0,0 +1,195 @@ | |||
| # Copyright 2019 The TensorFlow Authors. All Rights Reserved. | |||
| exchanges = 0 | ||
| num = tf.size(y) | ||
| k = tf.constant(1, tf.int32) | ||
| while tf.less(k, num): |
There was a problem hiding this comment.
Since TFP is a general purpose library, we need to write everything so that it does not presume eager execution. Minimally this means no python controlflow (eg, for, while, and if are not allowed).
More generally, this has a code nature that would be quite inefficient in a SIMD regime. Perhaps we could examine the full quadratic comparison? Eg,
0.5 * tf.math.reduce_mean(score[..., tf.newaxis] < score[..., tf.newaxis, :], axis=-1)
In general this would be quite large. I therefore recommend extending this idea to "chunks" using a tf.while_loop to reduce them. In so doing we can handle many cases in memory yet also bound the memory by way of chunks.
Thoughts?
There was a problem hiding this comment.
Only after I finished writing this did I fully examine the implementation of the AUC metrics in Keras, which use a bucketized approximation, not that dissimilar to the two-dimensional bucket approach presented in https://arxiv.org/abs/1712.01521 which would be much more amenable to streaming. (OTOH, I'm unclear why Metrics need to be streaming, usually the classifier labels is a space much smaller than the input features.)
I also note that I should have looked at the completely rewritten scipy implementation which works differently, but still requires sorting by both of the full inputs which seems largely incompatible with tf Metrics design.
https://github.com/scipy/scipy/blob/01d8bfb6f239df4ce70c799b9b485b53733c9911/scipy/stats/stats.py#L4452
I suspect a reasonable thing to do here is to probably fork this into two separate efforts, one for tfp which would focus on the explicit exact solution with potential backoff to approximate, and a tensorflow addons submission that focuses on implementing a streaming approximation. I think I can manage that, but it's going to take some time - and at present I don't have a good sense of which of these would be easier to do first.
There was a problem hiding this comment.
Hi - sorry I'm back to this finally - I think from the discussion this PR should be moved to tfp,
most likely in the https://github.com/tensorflow/probability/tree/master/tensorflow_probability/python/stats directory.
However, I think I need to make a significant redesign to comply with the suggestion of removing the control flow.
I'm not quite able to distill what the consensus is here for next steps, maybe I should do all of these:
- Make a tfp PR?
- Reimplement this as a streaming algorithm per the previously mentioned arxiv paper.
- Change this current PR to use tf.while(), possibly with chunks.
|
@jvdillon Thanks, I have question It seems that the TFP repository currently doesn't hosts metrics right? We could have case like this or our Matthews correlation impl where the user could need to consume it as inherited from |
|
We dont have keras metrics but we do have standalone functions which might
be regarded as metrics. We strive to be as library agnostic as possible. If
possible we prefer simple functions and have a strict policy that all
functions must support batch input. Take a look at other tfp.stats for
details.
…On Mon, Sep 21, 2020 at 4:35 PM bhack ***@***.***> wrote:
@jvdillon <https://github.com/jvdillon> Thanks, I have question It seems
that the TFP repository currently doesn't hosts metrics right? We could
have case like this or our Matthews correlation
<https://github.com/tensorflow/addons/blob/master/tensorflow_addons/metrics/matthews_correlation_coefficient.py>
impl where the user could need to consume it as inherited from
tf.keras.metrics.Metric object.
So if the ops is in TFP but the metric is here we need to depend on TFP.
Do you plan to support metrics directly?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2169 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIVTNWWRTQXH4R63APFN33SG7PLPANCNFSM4RQOFWYA>
.
|
|
@jvdillon Ok but I see that you have Keras layers and optimizers in TFP. |
|
On Tue, Sep 22, 2020 at 10:53 AM bhack ***@***.***> wrote:
@jvdillon <https://github.com/jvdillon> Ok but I see that you have Keras
layers and optimizers in TFP.
I think that it is ok to have an op like this in TFP but then why not to
have also metric that wraps the operator (when it is useful to be consumed
as metrics)?
I think that It will be hard for TF Addons to depend on TFP currently just
to implement metrics.
I will bring this question to the team. In the meantime, maybe proceed with
adding the core function?
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2169 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIVTNW3IOKABCQ4ICYTIWDSHDP7ZANCNFSM4RQOFWYA>
.
|
|
@jvdillon It is ok for me. P.s. Are you interested also in https://github.com/tensorflow/addons/blob/master/tensorflow_addons/metrics/matthews_correlation_coefficient.py? |
|
I guess keras metrics have some streaming functionality? At least the old tf.metrics did. |
@brianwa84 Yes, the cc: @seanpmorgan |
|
@seanpmorgan Before we lost this PR I am still oriented to include this TFP. |
|
@jvdillon @brianwa84 Do you have enough Github perm to migrate this in TFP? |
|
I don't know what you mean by "migrate". I think you can just open a new PR against TFP. For now, we could perhaps put something like this under |
Yes I supposed that we had a related issue about this feature as it was mandatory in our contribution policy. But in this case we have a PR directly.
The plan is ok for me @sorensenjs |
|
Created tensorflow/probability#1147, following up there with requested changes from this thread. |
|
This was added to tensorflow probability See https://github.com/tensorflow/probability/blob/master/tensorflow_probability/python/stats/kendalls_tau.py for more details. |
|
It would be nice if it could be exposed as metrics. |
|
Kendall's tau doesn't fit the metrics streaming style well. There are approximate versions (see https://arxiv.org/pdf/1712.01521.pdf) that are more interesting and well suited to the metrics. I might try doing that. |
Interesting. Thanks for the resoruce |
|
I've created a completely new streaming based algorithm for this abandoned PR |
Description
Adds Kendall's Tau, a measure of ordinal concordance as a Keras compatible metric,
Type of change
Checklist:
How Has This Been Tested?
Multiple tests have been included, and because this metric exists in non-tf form in scipy it's easy
to verify correctness.
If you're adding a bugfix or new feature please describe the tests that you ran to verify your changes:
*