Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docstring update: Clarify what "C-order" is #4452

Merged
merged 4 commits into from Apr 30, 2019
Merged

Docstring update: Clarify what "C-order" is #4452

merged 4 commits into from Apr 30, 2019

Conversation

@asmith26
Copy link
Contributor

@asmith26 asmith26 commented Feb 1, 2019

I didn't know what "C-order" was.After googling it I came across "column-major order" at Wikipedia.

  • [na] Tests added / passed
  • Passes flake8 dask
I didn't know what "C-order" was.After googling it I came across "column-major order" at [Wikipedia](https://en.wikipedia.org/wiki/Row-_and_column-major_order).
2. It only allows for reshapings that collapse or merge dimensions like
``(1, 2, 3, 4) -> (1, 6, 4)`` or ``(64,) -> (4, 4, 4)``
.. _`column-major order`: https://en.wikipedia.org/wiki/Row-_and_column-major_order
Copy link
Member

@TomAugspurger TomAugspurger Feb 1, 2019

I believe this line is too long for our linter. Do you know if sphinx lets you wrap links?

Copy link
Member

@martindurant martindurant Feb 1, 2019

an easier fix may be to shorten the label: _`cmo` ?

Copy link
Contributor Author

@asmith26 asmith26 Feb 1, 2019

Thanks for clarifying with me the issue.
I've recommitted and wrapped the link (example)

@TomAugspurger
Copy link
Member

@TomAugspurger TomAugspurger commented Feb 1, 2019

Still have a linting error: https://travis-ci.org/dask/dask/jobs/487619660#L2052

You can run flake8 locally FYI.

Do you know if sphinx will correctly render that link? Will it issue a warning?

It may be easier to shorten the text as @martindurant suggested.

@asmith26
Copy link
Contributor Author

@asmith26 asmith26 commented Feb 1, 2019

Still have a linting error: https://travis-ci.org/dask/dask/jobs/487619660#L2052

You can run flake8 locally FYI.

Thanks for highlighting where the error is, and for the tip about flake 8 - should be fixed this time.

Do you know if sphinx will correctly render that link? Will it issue a warning?

I built the doc locally with make html, and all looks good:

image

It may be easier to shorten the text as @martindurant suggested.

I'm happy either way.

@mrocklin
Copy link
Member

@mrocklin mrocklin commented Feb 1, 2019

@asmith26
Copy link
Contributor Author

@asmith26 asmith26 commented Feb 1, 2019

Is column-major the right term here? By C ordering we mean the ordering that the C language uses, wherein latter indices move fastest as we cycle through linear address space.

Thanks for clarification; in that case I actually think "row-major order" is more applicable here:
image

Column-major vs row-major order seems to be more applicable in the 2d matrix case?

I can see where you're coming from; indeed, to quote Wikipedia:

While the terms allude to the rows and columns of a two-dimensional array, i.e. a matrix, the orders can be generalized to arrays of any dimension by noting that the terms row-major and column-major are equivalent to lexicographic and colexicographic orders, respectively.

Thus, it could be more useful to clarify the order/manner that dask reshapes is the same as numpy's default setting (I never realised numpy's reshape actually had a "order" argument).

@jrbourbeau
Copy link
Member

@jrbourbeau jrbourbeau commented Feb 12, 2019

Thus, it could be more useful to clarify the order/manner that dask reshapes is the same as numpy's default setting

@asmith26 I agree that adding a note about Dask reshape assuming C-order, which is the NumPy reshape default, could help clarify things here. For what it's worth, the NumPy reshape documentation, which the Dask reshape documentation links to, has a nice description of the different order options.

@@ -136,10 +136,13 @@ def reshape(x, shape):
This is a parallelized version of the ``np.reshape`` function with the
following limitations:
1. It assumes that the array is stored in C-order
1. It assumes that the array is stored in `column-major order`_
Copy link
Member

@jakirkham jakirkham Feb 14, 2019

Maybe "stored in C-order (sometimes referred to as column-major order_)"?

Copy link
Member

@jrbourbeau jrbourbeau Feb 14, 2019

I think we decided C-order is row-major order. I like this suggestion, +1 for adding "(sometimes referred to as row-major order_)"

@mrocklin
Copy link
Member

@mrocklin mrocklin commented Apr 30, 2019

I've force pushed column -major to row-major. Merging.

@mrocklin mrocklin merged commit 566f7ab into dask:master Apr 30, 2019
jorge-pessoa pushed a commit to jorge-pessoa/dask that referenced this issue May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants