FIX euclidean_distances float32 numerical instabilities #13554

jeremiedbb · 2019-04-01T14:41:03Z

takes only the chunk upcast part of #13410: When input is float32, compute distances on chunks of X and Y upcast to float64.

memory usage of 10% of {X, Y, dist_matrix} with a min of 10Mib.

jnothman · 2019-04-01T20:30:39Z

Thank you @jeremiedbb. Some tests are failing. Could you please summarise why you are proposing this solution?

jeremiedbb · 2019-04-02T12:43:11Z

Well, this solution fixes the initial issue, i.e. precision issues in float32 (which was the most requested), while keeping good performance.

for low dimensional data, precision could be increased for float64 at no performance cost, but the cost would be severe for high dimensional data. Not mandatory for 0.21.
performance could be increased (and even be better than current) for float32 on low dimensional data. Not mandatory for 0.21.

Celelibi · 2019-04-02T15:36:04Z

sklearn/metrics/pairwise.py

+    maxmem = max(
+        ((x_density * n_samples_X + y_density * n_samples_Y) * n_features
+         + (x_density * n_samples_X * y_density * n_samples_Y)) / 10,
+        10 * 2**17)


Why this choice?
In my tests for #11271 I found that the computation time slowly increase with the memory size. It's the first few plots of this comment: #11271 (comment).
I'm mostly curious about this choice. I don't think this should prevent this PR from being merged.

A fixed 10Mib maxmem can lead to very small chunks when the number of features is very large (extreme case I agree). In that case the drop of performance can be quite severe.

Another possibility could be to have a fixed maxmem with a min chunk_size. I think it's kind of equivalent, but in this case you can have a memory increase bigger than expected whereas in the previous case the memory increase is bounded. That's why I went for the first one.

Large chunks would also decrease cache efficiency. Although I'm not a big fan of having an unbounded amount of additional memory, 10% of the memory already used is probably fair.

When I look at my old benchmarks, I think that there might be something going on under the hood. Maybe the optimal block size is actually constant. Moreover, if the number of features is large, it can also be chunked. That might deserve to be explored as well.
I think we can add to the todo-list to benchmark the influence of the memory size and chunking to get a proper understanding of the performance.

If the input data is memmapped, calculating this as a function of n_features may not be fair.

I think my comment about memmapping was incorrect in any case.

This logic seems to be more complicated than necessary, but okay.

I am fine to merge with this heuristic.

In the future, I am wondering if we could expose a chunk_size parameter which by default do such heuristic and could be fine-tuned by the user. @jeremiedbb mentioned to me that it might not be that easy since it will impact pairwise_distance where we would need to give also this parameter, etc. Just a thought.

rth · 2019-04-04T11:50:18Z

sklearn/metrics/pairwise.py

+    n_chunks_X = n_samples_X // chunk_size + (n_samples_X % chunk_size != 0)
+    n_chunks_Y = n_samples_Y // chunk_size + (n_samples_Y % chunk_size != 0)
+
+    for i in range(n_chunks_X):


Can we not use utils.gen_even_slices here?

gen_batches since we want a specific batch size, but it's cleaner indeed.

rth · 2019-04-04T11:56:51Z

sklearn/metrics/pairwise.py

+    distances = np.empty((n_samples_X, n_samples_Y), dtype=np.float32)
+
+    x_density = X.getnnz() / np.prod(X.shape) if issparse(X) else 1
+    y_density = Y.getnnz() / np.prod(Y.shape) if issparse(Y) else 1


Nit: X.nnz maybe

jnothman · 2019-04-06T12:31:51Z

doc/whats_new/v0.21.rst

+
+- |Fix| The function :func:`euclidean_distances`, and therefore the functions
+  :func:`pairwise_distances` and :func:`pairwise_distances_chunked` with
+  ``metric=euclidean``, suffered from numerical precision issues. Precision has


"with float32 features"

jnothman · 2019-04-06T12:32:11Z

doc/whats_new/v0.21.rst

+  in version 0.21 and will be removed in version 0.23. :issue:`10580` by
+  :user:`Reshama Shaikh <reshamas>` and :user:`Sandra Mitrovic <SandraMNE>`.
+
+- |Fix| The function :func:`euclidean_distances`, and therefore the functions


maybe more relevant: "and therefore several estimators with metric='euclidean'"

jnothman · 2019-04-06T12:32:43Z

doc/whats_new/v0.21.rst

+- |Fix| The function :func:`euclidean_distances`, and therefore the functions
+  :func:`pairwise_distances` and :func:`pairwise_distances_chunked` with
+  ``metric=euclidean``, suffered from numerical precision issues. Precision has
+  been increased for float32. :issue:`13554` by :user:` <Celelibi>` and


I think correct syntax is just :user:`Celelibi`

jnothman · 2019-04-06T12:33:35Z

sklearn/metrics/pairwise.py


    return distances if squared else np.sqrt(distances, out=distances)


+def _euclidean_distances_upcast_fast(X, XX=None, Y=None, YY=None):


"_fast" is a bit obscure

it's a residue of my previous PR implementing fast and exact method. I removed it.

jnothman · 2019-04-06T14:09:37Z

sklearn/metrics/pairwise.py

+    maxmem = max(
+        ((x_density * n_samples_X + y_density * n_samples_Y) * n_features
+         + (x_density * n_samples_X * y_density * n_samples_Y)) / 10,
+        10 * 2**17)


If the input data is memmapped, calculating this as a function of n_features may not be fair.

jnothman · 2019-04-06T14:10:59Z

sklearn/metrics/pairwise.py

+         + (x_density * n_samples_X * y_density * n_samples_Y)) / 10,
+        10 * 2**17)
+
+    # The increase amount of memory is:


add "in 8-byte blocks"... in fact where do you account for this?

maxmem is the max allowed number of 8-byte blocks for extra memory usage.
10Mib is 10 * 2**20 bits = 10 * 2**17 8-byte blocks.

jeremiedbb · 2019-04-12T12:11:13Z

If the input data is memmapped, calculating this as a function of n_features may not be fair.

Yes that's true. But having a fixed 10Mib can be very bad when the number of features is high because in that case the chunk size is very small.

Do you think 10Mib + a min chunk size would be better (Note that the 10Mib is not guaranteed in that case) ?

jnothman

I think this is looking good enough to merge.

jnothman · 2019-04-15T01:00:22Z

sklearn/metrics/pairwise.py

+    maxmem = max(
+        ((x_density * n_samples_X + y_density * n_samples_Y) * n_features
+         + (x_density * n_samples_X * y_density * n_samples_Y)) / 10,
+        10 * 2**17)


I think my comment about memmapping was incorrect in any case.

doc/whats_new/v0.21.rst

jnothman · 2019-04-15T01:03:04Z

sklearn/metrics/pairwise.py

@@ -231,34 +231,115 @@ def euclidean_distances(X, Y=None, Y_norm_squared=None, squared=False,
        elif XX.shape != (X.shape[0], 1):
            raise ValueError(
                "Incompatible dimensions for X and X_norm_squared")
+        if XX.dtype == np.float32:
+            XX = None


should we raise a warning that these cannot be used??

Why not but I'm afraid it could generate a lot of warnings. For instance when pairwise_distances is called in a loop.

One thing we should certainly do is adding an option to row_norms to use a float64 accumulator. That's on my todo list :)

jnothman · 2019-04-15T01:07:04Z

sklearn/metrics/pairwise.py

    np.maximum(distances, 0, out=distances)

+    # Ensure that distances between vectors and themselves are set to 0.0.


why is this out of the if? It's only applicable if the if holds.

(Curiously we currently appear to duplicate this logic in pairwise_distances_chunked but not in pairwise_distances)

you mean zeroing the diagonal ? It applies using both methods.

It's necessary to also do it in pairwise_distances_chunked, because we lose the diagonal info when passing chunks to pairwise_distances.

Yes, but I don't think we zero the diagonal in pairwise_distances at all for other metrics

I was looking at euclidean_distances, not pairwise_distances... That's strange indeed. Even for metric=euclidean it's not enforced when n_jobs > 1. Let me put that also on my todo list :)

jnothman · 2019-04-15T01:09:33Z

sklearn/metrics/pairwise.py

+    maxmem = max(
+        ((x_density * n_samples_X + y_density * n_samples_Y) * n_features
+         + (x_density * n_samples_X * y_density * n_samples_Y)) / 10,
+        10 * 2**17)


This logic seems to be more complicated than necessary, but okay.

thomasjpfan · 2019-04-15T13:39:55Z

sklearn/metrics/pairwise.py

+    # - x_density * batch_size * n_features (copy of chunk of X)
+    # - y_density * batch_size * n_features (copy of chunk of Y)
+    # - batch_size * batch_size (chunk of distance matrix)
+    # Hence x² + (xd+yd)kx = M, where x=batch_size, k=n_features, M=maxmem


When a grade school student ask, "Why are we learning the quadratic equation?", we have an answer.

thomasjpfan · 2019-04-15T13:43:19Z

sklearn/metrics/pairwise.py

+        if YY.dtype == np.float32:
+            YY = None
+    elif Y.dtype == np.float32:
+        YY = None


At first glance, it is not obvious that None is used by _euclidean_distances_upcast.

What do you mean ? XX and YY are used by _euclidean_distances_upcast no matter their value

Setting XX or YY to None tells _euclidean_distances_upcast to call row_norms(*_chunk, squared=True)[:, np.newaxis].

Without reading _euclidean_distances_upcast, it is difficult to tell why XX or YY are set to None.

I think that we would need a small comment to clarify this part.

thomasjpfan · 2019-04-15T13:54:00Z

sklearn/metrics/tests/test_pairwise.py

+                          (np.float64, 1e-8)])
+@pytest.mark.parametrize("dim", [1, 1000000])
+def test_euclidean_distances_extreme_values(dtype, s, dim):
+    # check that euclidean distances is correct where 'fast' method wouldn't be


fast method?

I made the comment clearer

jnothman · 2019-04-21T10:19:00Z

Are we going to get this into 0.21?

jnothman · 2019-04-23T15:03:22Z

Are you able to look at this, @qinhanmin2014?

qinhanmin2014 · 2019-04-23T15:31:21Z

Are you able to look at this, @qinhanmin2014?

Apologies I won't be available until this weekend (will try to merge OPTICS these days).

jnothman · 2019-04-24T13:24:17Z

@rth are you okay with this?

rth · 2019-04-25T08:49:04Z

Are we going to get this into 0.21?

It would good I think.

Will review this in the next couple of days.

glemaitre

LGTM. Only nitpicks.

doc/whats_new/v0.21.rst

glemaitre · 2019-04-25T09:44:08Z

sklearn/metrics/pairwise.py

@@ -203,7 +203,7 @@ def euclidean_distances(X, Y=None, Y_norm_squared=None, squared=False,

    Returns
    -------
-    distances : {array, sparse matrix}, shape (n_samples_1, n_samples_2)
+    distances : array, shape (n_samples_1, n_samples_2)


I put the comment here but I think that it should be written above.

I would either add a note (using sphinx syntax) or within the parameter description that X_norm_squared and Y_norm_squared will be ignored if they are passed as float32 since they will lead to inaccurate distances.

glemaitre · 2019-04-25T09:44:45Z

sklearn/metrics/pairwise.py

+        if YY.dtype == np.float32:
+            YY = None
+    elif Y.dtype == np.float32:
+        YY = None


I think that we would need a small comment to clarify this part.

glemaitre · 2019-04-25T09:45:50Z

sklearn/metrics/pairwise.py

+    Assumes XX and YY have float64 dtype or are None.
+
+    X and Y are upcast to float64 by chunks, which size is chosen to limit
+    memory increase by approximately 10%.


Suggested change

memory increase by approximately 10%.

memory increase by approximately 10% (bounded to 10Mb).

glemaitre · 2019-04-25T09:50:58Z

sklearn/metrics/pairwise.py

+    maxmem = max(
+        ((x_density * n_samples_X + y_density * n_samples_Y) * n_features
+         + (x_density * n_samples_X * y_density * n_samples_Y)) / 10,
+        10 * 2**17)


I am fine to merge with this heuristic.

In the future, I am wondering if we could expose a chunk_size parameter which by default do such heuristic and could be fine-tuned by the user. @jeremiedbb mentioned to me that it might not be that easy since it will impact pairwise_distance where we would need to give also this parameter, etc. Just a thought.

glemaitre · 2019-04-25T09:55:01Z

sklearn/metrics/tests/test_pairwise.py

+    Y = y_array_constr(Y)
+    distances = euclidean_distances(X, Y)
+
+    assert_allclose(distances, expected, rtol=1e-6)


a small comments regarding rtol=1e-6

glemaitre · 2019-04-25T09:55:50Z

sklearn/metrics/tests/test_pairwise.py

+    assert distances.dtype == dtype
+
+
+@pytest.mark.parametrize("dtype, s",


Maybe we could have a better name for s

glemaitre · 2019-04-25T09:57:12Z

sklearn/metrics/tests/test_pairwise.py

+
+@pytest.mark.parametrize("dtype, s",
+                         [(np.float32, 1e-4),
+                          (np.float64, 1e-8)])


Suggested change

(np.float64, 1e-8)])

pytest.param(np.float64, 1e-8, marks=pytest.mark.xfail('failing due to lack of precision'))])

…cast

jeremiedbb · 2019-04-25T15:29:25Z

I messed up :/ but the diff is correct

jnothman · 2019-04-27T14:05:44Z

We should take that review as Approve, @glemaitre?

rth

Thanks to work working on this @jeremiedbb, and for all the insightful comments/discussions by reviewers.

Overall looks good to me. Below are a few comments mostly about readability..

rth · 2019-04-27T14:57:21Z

sklearn/metrics/pairwise.py

+                d += XX_chunk
+                d += YY_chunk
+
+            distances[x_slice, y_slice] = d.astype(np.float32)


Add copy=False, in the case when d = distances[y_slice, x_slice].T it is already float32, and astype make a copy by default.

this function only computes euclidean distances on float64 and downcast it to float32 at the end so actually there will always be a copy. Actually I find it more clear to not add the copy=False parameter to emphasize that

rth · 2019-04-27T15:06:11Z

sklearn/metrics/pairwise.py

+            if X is Y and j < i:
+                # when X is Y the distance matrix is symmetric so we only need
+                # to compute half of it.
+                d = distances[y_slice, x_slice].T


It must be correct since the corresponding tests pass, but after thinking about it for 20s I'm still not confident that when we compute distances[x_slice, y_slice], distances[y_slice, x_slice] is already computed, is not null and can be copied from.

How about just adding a continue here, then add a second loop below,

if X is Y: # the result is symmetric, copy distances from the lower triangular matrix for i, x_slice in enumerate(x_batches): for j, y_slice in enumerate(y_batches): if j < i: distances[x_slice, y_slice] = distances[x_slice, y_slice].T

it shouldn't matter for run time too much time I think, but it might make things a bit more explicit?

still not confident that when we compute distances[x_slice, y_slice], distances[y_slice, x_slice] is already computed

It is since we compute the chunks with i (rows) as the outer loop. It implies that all chunks of the upper right triangular part of the distance matrix are computed before their symmetric chunk in the lower left triangular part.

rth · 2019-04-27T15:19:14Z

sklearn/metrics/pairwise.py

+    y_density = Y.nnz / np.prod(Y.shape) if issparse(Y) else 1
+
+    # Allow 10% more memory than X, Y and the distance matrix take (at least
+    # 10Mib)


Can we use MiB (or MB) as all other cache sizes in scikit-learn? Mib unit can lead to confusion when comparing to the CPU L3 cache size or to working_memory.

rth · 2019-04-27T15:38:46Z

sklearn/metrics/pairwise.py

+    # 10Mib)
+    maxmem = max(
+        ((x_density * n_samples_X + y_density * n_samples_Y) * n_features
+         + (x_density * n_samples_X * y_density * n_samples_Y)) / 10,


This makes the assumption that sizeof(dtype) == 8 without making it appear in the computation, I think?

Maybe we could add X.dtype.itemsize / 8 even if that is equal to 1, to make it easier to follow what is going on.

Also not that it matters too much, but

for dense isn't that its 5% of (X, Y, distance) as those are float32. i.e. X.nbytes == x_density * n_samples_X * 4 / 8?

for sparse it's a roughly 10% if it is CSR and we account for X.data, X.indptr in 32 bit, but again it's far from obvious for a casual reader.

Actually, maybe,

def get_array_nbytes(X): if issparse(X): if hasattr(X, 'indices'): # CSR or CSC return X.data.nbytes + X.data.nbytes + X.data.indptr else: # some other sparse format, assume 8 bytes to index # one non zero element (e.g. COO) return X.data.nbytes + X.nnz*8 else: return X.nbytes maxmem = (get_array_size(X) + get_array_size(Y) + get_array_size(distances)) / 10

This makes the assumption that sizeof(dtype) == 8 without making it appear in the computation, I think?

This function is only used for float32 X and Y. I've explained it in its docstring.

for sparse it's a roughly 10% if it is CSR and we account for X.data, X.indptr in 32 bit, but again it's far from obvious for a casual reader.

When you change the type of the csr matrix, only a copy of data is made. indices and indptr are still int, so I think this formula is correct

When you change the type of the csr matrix, only a copy of data is made.

Are you sure?

>>> import scipy.sparse >>> import numpy as nop >>> import numpy as np >>> X = scipy.sparse.csr_matrix(np.ones((10, 10))) >>> X.dtype dtype('float64') >>> Y = X.astype(np.float32) >>> np.shares_memory(Y.data, Y.data) True >>> np.shares_memory(Y.data, X.data) False >>> np.shares_memory(Y.indices, X.indices) False

That's not critical, my point here is that it might be beter (not necessarily in this PR) to use ndarray.nbytes than trying to re-compute that from scratch. If we get this calculation wrong we won't likely know, since no test will fail and we will only reason in wrong sizes of memory cache.

sklearn/metrics/pairwise.py

glemaitre · 2019-04-28T09:37:40Z

Remaining the comments of @rth

jnothman

I made the astype(copy=False) change under the assumption that this is the only substantive change blocking merge.

rth · 2019-04-29T04:11:37Z

I made the astype(copy=False) change under the assumption that this is the only substantive change blocking merge.

Thanks, feel free to merge if this PR is blocking the release.

jnothman · 2019-04-29T05:07:20Z

I consider #13739 to be the blocker.

jeremiedbb · 2019-04-29T07:47:35Z

I'm ok letting astype(copy=False), but I want to remind that this function is only used on float32 data so the copy will always be made, and I find more confusing since it may give the impression that it's possible to have no copy.

jnothman · 2019-04-29T07:51:39Z

So then remove the copy=True. Thanks. Happy to merge when you think you have addressed Roman's comments.

jeremiedbb · 2019-04-29T08:15:58Z

I think I have. @rth do you agree with my answers to your comments ?

rth · 2019-04-29T08:34:30Z

but I want to remind that this function is only used on float32 data so the copy will always be made,

In general a copy will be made, but when X is Y, on the upper triangular matrix, this used to do,

distances[x_slice, y_slice] = distances[y_slice, x_slice].astype(np.float32)

which does make 1 extra unnecessary copy since distances is already float32.

and I find more confusing since it may give the impression that it's possible to have no copy.

Yes, hence my comments about splitting that part of code a bit more. But it's not critical.

jeremiedbb · 2019-04-29T08:39:13Z

you're right. I put the copy=False back :)

rth

Last comment apart from that LGTM.

sklearn/metrics/pairwise.py

rth · 2019-04-29T09:03:28Z

Merging. Thanks @jeremiedbb and @Celelibi !

…#13554)

jeremiedbb added 6 commits March 29, 2019 18:11

vect vect euclidean

d38d8a0

minmem

01767d6

docstring

83cada9

lint

ecb3d2c

unrelated

6b140db

update tests + fix int

558b988

jeremiedbb mentioned this pull request Apr 1, 2019

[MRG] Fix euclidean_distances numerical instabilities #13410

Closed

fix chunk size

de9d217

fix sparse slice bounds

395be7a

fix what's new

9c79570

Celelibi reviewed Apr 2, 2019

View reviewed changes

rth reviewed Apr 4, 2019

View reviewed changes

jeremiedbb added 3 commits April 5, 2019 16:20

tmp

4599379

use gen_batches

7425821

nitpick

a2504af

jnothman reviewed Apr 6, 2019

View reviewed changes

adress what's comments + clean naming

fee1258

jnothman approved these changes Apr 15, 2019

View reviewed changes

thomasjpfan reviewed Apr 15, 2019

View reviewed changes

clearer comment

fe74973

jeremiedbb force-pushed the euclidean_dist_upcast branch from 2eca804 to fe74973 Compare April 15, 2019 14:04

glemaitre requested changes Apr 25, 2019

View reviewed changes

jeremiedbb added 3 commits April 25, 2019 16:47

merge

32c9f99

adress comments

0f346f5

Merge remote-tracking branch 'upstream/master' into euclidean_dist_up…

fce0713

…cast

rth reviewed Apr 27, 2019

View reviewed changes

glemaitre approved these changes Apr 28, 2019

View reviewed changes

astype=False per Roman's comment

117529e

jnothman reviewed Apr 29, 2019

View reviewed changes

Mib -> MiB

9c205c1

back to copy=False

985037a

rth approved these changes Apr 29, 2019

View reviewed changes

sklearn/metrics/pairwise.py Show resolved Hide resolved

rth changed the title ~~[MRG] Fix euclidean_distances float32 numerical instabilities~~ FIX euclidean_distances float32 numerical instabilities Apr 29, 2019

rth merged commit f16227b into scikit-learn:master Apr 29, 2019

marcelobeckmann pushed a commit to marcelobeckmann/scikit-learn that referenced this pull request May 1, 2019

FIX euclidean_distances float32 numerical instabilities (scikit-learn…

8cc70af

…#13554)

marcelobeckmann pushed a commit to marcelobeckmann/scikit-learn that referenced this pull request May 1, 2019

FIX euclidean_distances float32 numerical instabilities (scikit-learn…

cc0184f

…#13554)

rth mentioned this pull request May 18, 2019

Untreated overflow (?) for float32 in euclidean_distances new in sklearn 21.1 #13905

Closed

gokceneraslan mentioned this pull request May 21, 2019

Scanpy not working correctly with scikit-learn 0.21.1 scverse/scanpy#654

Closed

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

FIX euclidean_distances float32 numerical instabilities (scikit-learn…

a28577d

…#13554)

This was referenced Aug 5, 2019

check_symmetric: adapt tolerance to data type #11159

Closed

Fixed adapting tolerance value according to data type in manifold MDS #11167

Closed


		return distances if squared else np.sqrt(distances, out=distances)


		def _euclidean_distances_upcast_fast(X, XX=None, Y=None, YY=None):

		np.maximum(distances, 0, out=distances)

		# Ensure that distances between vectors and themselves are set to 0.0.

	memory increase by approximately 10%.
	memory increase by approximately 10% (bounded to 10Mb).

		assert distances.dtype == dtype


		@pytest.mark.parametrize("dtype, s",

	(np.float64, 1e-8)])
	pytest.param(np.float64, 1e-8, marks=pytest.mark.xfail('failing due to lack of precision'))])

FIX euclidean_distances float32 numerical instabilities #13554

FIX euclidean_distances float32 numerical instabilities #13554

Conversation

jeremiedbb commented Apr 1, 2019

jnothman commented Apr 1, 2019

jeremiedbb commented Apr 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremiedbb commented Apr 12, 2019 • edited

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Apr 21, 2019

jnothman commented Apr 23, 2019

qinhanmin2014 commented Apr 23, 2019 • edited

jnothman commented Apr 24, 2019

rth commented Apr 25, 2019

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremiedbb commented Apr 25, 2019

jnothman commented Apr 27, 2019

rth left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rth Apr 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rth Apr 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre commented Apr 28, 2019

jnothman left a comment

Choose a reason for hiding this comment

rth commented Apr 29, 2019

jnothman commented Apr 29, 2019 via email

jeremiedbb commented Apr 29, 2019

jnothman commented Apr 29, 2019 via email

jeremiedbb commented Apr 29, 2019

rth commented Apr 29, 2019 • edited

jeremiedbb commented Apr 29, 2019

jeremiedbb commented Apr 12, 2019 •

edited

qinhanmin2014 commented Apr 23, 2019 •

edited

rth left a comment •

edited

rth Apr 27, 2019 •

edited

rth Apr 27, 2019 •

edited

rth commented Apr 29, 2019 •

edited