Minor punctuation fix

[ClimbsRocks]

Minor comma update to separate clauses in this sentence, and make it slightly more clear.

[jnothman]

hmm. Should that first comma come after "and"?

[nelson-liu]

i personally think it'd sound better as:
Internally, it will be converted to dtype=np.float32, but to a sparse csr_matrix if a sparse matrix is provided.

the and is sort of weird, since you're only doing one or the other...

[lesteve]

the and is sort of weird, since you're only doing one or the other...

Quickly looking at the code, dtype='float32' is always used, whether you provide an array or a sparse matrix. On top of that, if you do provide a sparse matrix, it will be converted into csr. So basically the "and" is kind of right.

Here is my attempt:
Internally, it will be converted to dtype=np.float32. If a sparse matrix is provided, it will be converted into a sparse csr_matrix.

Not a native speaker, so no strong feeling on this. Whichever phrasing is deemed best it would be great if it was updated in a consistent manner in all the files.

❯ git grep -l 'and if a sparse'
sklearn/ensemble/forest.py
sklearn/ensemble/gradient_boosting.py
sklearn/ensemble/iforest.py
sklearn/tree/tree.py

[nelson-liu]

@lesteve ah ok, i see how that works. in that case, I agree with @jnothman 's opinion that the first comma should come after the "and".

[lesteve]

in that case, I agree with @jnothman 's opinion that the first comma should come after the "and".

I find my attempt with two sentences a bit easier to parse TBH and it has the clear advantage of cutting short any discussion about where to put the comma. What about this?

Internally, its dtype will be converted to np.float32. If a sparse matrix is provided, it will be converted into a sparse csr_matrix.

[ClimbsRocks]

I agree with the two sentence approach. And with changing the wording to be consistent across all the relevant files. I'll update the PR to reflect that shortly once I'm waiting for a few models to train.

Thanks for all the interest in clarity, everyone!

[ClimbsRocks]

I went with both of @lesteve 's suggestions: The two sentence approach, and fixing this everywhere that sentence appears. Thanks for checking into this with me!

[lesteve]

Shouldn't it be "its dtype" (here and in every similar sentence below) ?

[ClimbsRocks]

Thanks for catching that! I'll update in a bit when I'm back at a computer.

On Aug 27, 2016 4:42 PM, "Loïc Estève" notifications@github.com wrote:

In sklearn/ensemble/forest.py
#6926 (comment)
:

@@ -160,9 +160,9 @@ def apply(self, X):
Parameters
----------
X : array-like or sparse matrix, shape = [n_samples, n_features]

•        The input samples. Internally, it will be converted to
  

•        `dtype=np.float32` and if a sparse matrix is provided
  

•        to a sparse `csr_matrix`.
  

•        The input samples. Internally, it's dtype will be converted to
  

Shouldn't it be "its dtype" (here and in every similar sentence below) ?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/scikit-learn/scikit-learn/pull/6926/files/55cf091aa6130a7bea0cb554f6e50500013f10f0#r76522723,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGsSVaBDKRMJMW3Ae7Y7YJvsesCR14l_ks5qkK9OgaJpZM4I8Sk4
.

[amueller]

lgtm

[lesteve]

LGTM, merging, thanks a lot!