ARROW-9967: [Python] Add compute module docs + expose more option classes - #8145

Closed
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871
Closed

ARROW-9967: [Python] Add compute module docs + expose more option classes#8145
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871

Conversation

@arw2019

@arw2019arw2019 commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

#8163 exposes pyarrow.compute kernels and generates their docstrings. This PR adds documentation for the module in the User Guide and the Python API reference.

@arw2019
arw2019 marked this pull request as draft September 9, 2020 04:22
@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@arw2019arw2019 changed the title WIP: ARROW-7871: [Python] [DOC] Expose more compute kernels and add compute module docsARROW-9967: [Python] Add compute module docsSep 10, 2020
@github-actions

Copy link
Copy Markdown

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from edb57cc to 3f99ae8CompareSeptember 13, 2020 03:18
@arw2019
arw2019 marked this pull request as ready for review September 13, 2020 03:18
@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module docsARROW-9967: [Python] Add compute module documentationSep 13, 2020
Comment threaddocs/source/cpp/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that this was missing on the c++ page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you! However, can you leave the table alphabetically-sorted?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my bad, you already had it there. Reverted

@jorisvandenbossche

Copy link
Copy Markdown
Member

@arw2019 thanks a lot for working on this! (and sorry for the overlap with #8163)

Comment threaddocs/source/python/api/compute.rst Outdated
Comment threaddocs/source/python/api/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really a transform?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair. Would Structural Transforms be a good place for it?

Comment threaddocs/source/python/compute.rst Outdated
Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this array is not used? (maybe show pc.equals(a, b) ?)

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from e79559c to d117ed1CompareSeptember 15, 2020 04:10
Comment threadpython/pyarrow/compute.py Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche@pitrou

AFAIK we can't have methods named and and or in Python. For now I went with and_ and or_ here, for consistency with and_kleene and or_kleene, but for example numpy uses np.logical_and and np.logical_or

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a function named "and":

>>>frompyarrowimportcompute>>>getattr(compute, "and")
<functionpyarrow.compute.and(left, right, *, memory_pool=None)>

So you should just fix compute.py so that it's exported as and_.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed now

Comment threaddocs/source/python/api/compute.rst Outdated

@pitroupitrouSep 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this render the docstrings?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. It's what's done on the other pages:
https://github.com/apache/arrow/blob/master/docs/source/python/api/datatypes.rst

I'm having trouble building the docs locally (related to ARROW-10018 I think) so haven't been able to check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't render the docstring here, but it will create a separate page for each of those functions and link to that from this table.

(the separate pages might be a bit overkill since the docstrings here are often not that informative yet, but it's indeed how we do it for other functions as well)

@arw2019
arw2019force-pushed the ARROW-7871 branch 5 times, most recently from c2445c1 to c1c5846CompareSeptember 16, 2020 20:26
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Question: in 952649d I'm attempting to expose PartitionNthOptions but it errors at compilation. What is (are) my mistake(s)?

This is needed to make the partition_nth_indices Python wrapper usable.

@pitrou

Copy link
Copy Markdown
Member

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly. Also, it is better to use __cinit__ here so that the constructor never gets skipped, so for example (untested):

def__cinit__(self, int64_t pivot):
self.partition_nth_options.reset(new CPartitionNthOptions(pivot))

@arw2019

Copy link
Copy Markdown
ContributorAuthor

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly.

Thanks @pitrou! That was the problem

@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module documentationARROW-9967: [Python] Add compute module docs + expose more option classesSep 22, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

This is ready for re-review.

I believe that I've addressed the feedback from previous reviews. I've also now exposed all the option classes so that all the kernels listed in the docs are usable from Python.

Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche Will this work? I'm not 100% sure as I haven't yet compiled the docs to explicitly check

Comment threadpython/pyarrow/_compute.pyx Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why unique_ptr is used in some of the other classes, but it shouldn't be necessary. See MinMaxOptions for an example without.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PartitionNthOptions we need it because it doesn't have a null constructor so it can't be allocated on the stack. That's not the case for VarianceOptions, though, so I'll take a look

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewritten VarianceOptions without unique_ptr.

I think that with the other three options classes exposed in this PR (SetLookupOptions, PartitionNthOptions and StrptimeOptions) it's necessary to use unique_ptr or, alternatively, we would need to define default constructors

@kszucs

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-ubuntu-18.04-docs

@github-actions

Copy link
Copy Markdown

Revision: 73272bf38f2d8406b60a6ebaa20c5f4e58dff075

Submitted crossbow builds: ursa-labs/crossbow @ actions-615

TaskStatus
test-ubuntu-18.04-docsAzure

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Will merge after a couple changes. Thank you @arw2019 !

@pitrou

Copy link
Copy Markdown
Member

CI failures are unrelated.

@pitroupitrou closed this in ba7ee65Oct 8, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Thanks @pitrou@jorisvandenbossche for reviewing!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@arw2019@jorisvandenbossche@pitrou@kszucs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

ARROW-9967: [Python] Add compute module docs + expose more option classes - #8145

Closed
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871
Closed

ARROW-9967: [Python] Add compute module docs + expose more option classes#8145
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871

Conversation

@arw2019

@arw2019arw2019 commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

#8163 exposes pyarrow.compute kernels and generates their docstrings. This PR adds documentation for the module in the User Guide and the Python API reference.

@arw2019
arw2019 marked this pull request as draft September 9, 2020 04:22
@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@arw2019arw2019 changed the title WIP: ARROW-7871: [Python] [DOC] Expose more compute kernels and add compute module docsARROW-9967: [Python] Add compute module docsSep 10, 2020
@github-actions

Copy link
Copy Markdown

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from edb57cc to 3f99ae8CompareSeptember 13, 2020 03:18
@arw2019
arw2019 marked this pull request as ready for review September 13, 2020 03:18
@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module docsARROW-9967: [Python] Add compute module documentationSep 13, 2020
Comment threaddocs/source/cpp/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that this was missing on the c++ page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you! However, can you leave the table alphabetically-sorted?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my bad, you already had it there. Reverted

@jorisvandenbossche

Copy link
Copy Markdown
Member

@arw2019 thanks a lot for working on this! (and sorry for the overlap with #8163)

Comment threaddocs/source/python/api/compute.rst Outdated
Comment threaddocs/source/python/api/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really a transform?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair. Would Structural Transforms be a good place for it?

Comment threaddocs/source/python/compute.rst Outdated
Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this array is not used? (maybe show pc.equals(a, b) ?)

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from e79559c to d117ed1CompareSeptember 15, 2020 04:10
Comment threadpython/pyarrow/compute.py Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche@pitrou

AFAIK we can't have methods named and and or in Python. For now I went with and_ and or_ here, for consistency with and_kleene and or_kleene, but for example numpy uses np.logical_and and np.logical_or

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a function named "and":

>>>frompyarrowimportcompute>>>getattr(compute, "and")
<functionpyarrow.compute.and(left, right, *, memory_pool=None)>

So you should just fix compute.py so that it's exported as and_.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed now

Comment threaddocs/source/python/api/compute.rst Outdated

@pitroupitrouSep 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this render the docstrings?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. It's what's done on the other pages:
https://github.com/apache/arrow/blob/master/docs/source/python/api/datatypes.rst

I'm having trouble building the docs locally (related to ARROW-10018 I think) so haven't been able to check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't render the docstring here, but it will create a separate page for each of those functions and link to that from this table.

(the separate pages might be a bit overkill since the docstrings here are often not that informative yet, but it's indeed how we do it for other functions as well)

@arw2019
arw2019force-pushed the ARROW-7871 branch 5 times, most recently from c2445c1 to c1c5846CompareSeptember 16, 2020 20:26
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Question: in 952649d I'm attempting to expose PartitionNthOptions but it errors at compilation. What is (are) my mistake(s)?

This is needed to make the partition_nth_indices Python wrapper usable.

@pitrou

Copy link
Copy Markdown
Member

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly. Also, it is better to use __cinit__ here so that the constructor never gets skipped, so for example (untested):

def__cinit__(self, int64_t pivot):
self.partition_nth_options.reset(new CPartitionNthOptions(pivot))

@arw2019

Copy link
Copy Markdown
ContributorAuthor

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly.

Thanks @pitrou! That was the problem

@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module documentationARROW-9967: [Python] Add compute module docs + expose more option classesSep 22, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

This is ready for re-review.

I believe that I've addressed the feedback from previous reviews. I've also now exposed all the option classes so that all the kernels listed in the docs are usable from Python.

Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche Will this work? I'm not 100% sure as I haven't yet compiled the docs to explicitly check

Comment threadpython/pyarrow/_compute.pyx Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why unique_ptr is used in some of the other classes, but it shouldn't be necessary. See MinMaxOptions for an example without.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PartitionNthOptions we need it because it doesn't have a null constructor so it can't be allocated on the stack. That's not the case for VarianceOptions, though, so I'll take a look

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewritten VarianceOptions without unique_ptr.

I think that with the other three options classes exposed in this PR (SetLookupOptions, PartitionNthOptions and StrptimeOptions) it's necessary to use unique_ptr or, alternatively, we would need to define default constructors

@kszucs

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-ubuntu-18.04-docs

@github-actions

Copy link
Copy Markdown

Revision: 73272bf38f2d8406b60a6ebaa20c5f4e58dff075

Submitted crossbow builds: ursa-labs/crossbow @ actions-615

TaskStatus
test-ubuntu-18.04-docsAzure

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Will merge after a couple changes. Thank you @arw2019 !

@pitrou

Copy link
Copy Markdown
Member

CI failures are unrelated.

@pitroupitrou closed this in ba7ee65Oct 8, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Thanks @pitrou@jorisvandenbossche for reviewing!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@arw2019@jorisvandenbossche@pitrou@kszucs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-9967: [Python] Add compute module docs + expose more option classes - #8145

Closed
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871
Closed

ARROW-9967: [Python] Add compute module docs + expose more option classes#8145
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871

Conversation

@arw2019

@arw2019arw2019 commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

#8163 exposes pyarrow.compute kernels and generates their docstrings. This PR adds documentation for the module in the User Guide and the Python API reference.

@arw2019
arw2019 marked this pull request as draft September 9, 2020 04:22
@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@arw2019arw2019 changed the title WIP: ARROW-7871: [Python] [DOC] Expose more compute kernels and add compute module docsARROW-9967: [Python] Add compute module docsSep 10, 2020
@github-actions

Copy link
Copy Markdown

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from edb57cc to 3f99ae8CompareSeptember 13, 2020 03:18
@arw2019
arw2019 marked this pull request as ready for review September 13, 2020 03:18
@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module docsARROW-9967: [Python] Add compute module documentationSep 13, 2020
Comment threaddocs/source/cpp/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that this was missing on the c++ page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you! However, can you leave the table alphabetically-sorted?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my bad, you already had it there. Reverted

@jorisvandenbossche

Copy link
Copy Markdown
Member

@arw2019 thanks a lot for working on this! (and sorry for the overlap with #8163)

Comment threaddocs/source/python/api/compute.rst Outdated
Comment threaddocs/source/python/api/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really a transform?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair. Would Structural Transforms be a good place for it?

Comment threaddocs/source/python/compute.rst Outdated
Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this array is not used? (maybe show pc.equals(a, b) ?)

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from e79559c to d117ed1CompareSeptember 15, 2020 04:10
Comment threadpython/pyarrow/compute.py Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche@pitrou

AFAIK we can't have methods named and and or in Python. For now I went with and_ and or_ here, for consistency with and_kleene and or_kleene, but for example numpy uses np.logical_and and np.logical_or

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a function named "and":

>>>frompyarrowimportcompute>>>getattr(compute, "and")
<functionpyarrow.compute.and(left, right, *, memory_pool=None)>

So you should just fix compute.py so that it's exported as and_.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed now

Comment threaddocs/source/python/api/compute.rst Outdated

@pitroupitrouSep 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this render the docstrings?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. It's what's done on the other pages:
https://github.com/apache/arrow/blob/master/docs/source/python/api/datatypes.rst

I'm having trouble building the docs locally (related to ARROW-10018 I think) so haven't been able to check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't render the docstring here, but it will create a separate page for each of those functions and link to that from this table.

(the separate pages might be a bit overkill since the docstrings here are often not that informative yet, but it's indeed how we do it for other functions as well)

@arw2019
arw2019force-pushed the ARROW-7871 branch 5 times, most recently from c2445c1 to c1c5846CompareSeptember 16, 2020 20:26
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Question: in 952649d I'm attempting to expose PartitionNthOptions but it errors at compilation. What is (are) my mistake(s)?

This is needed to make the partition_nth_indices Python wrapper usable.

@pitrou

Copy link
Copy Markdown
Member

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly. Also, it is better to use __cinit__ here so that the constructor never gets skipped, so for example (untested):

def__cinit__(self, int64_t pivot):
self.partition_nth_options.reset(new CPartitionNthOptions(pivot))

@arw2019

Copy link
Copy Markdown
ContributorAuthor

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly.

Thanks @pitrou! That was the problem

@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module documentationARROW-9967: [Python] Add compute module docs + expose more option classesSep 22, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

This is ready for re-review.

I believe that I've addressed the feedback from previous reviews. I've also now exposed all the option classes so that all the kernels listed in the docs are usable from Python.

Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche Will this work? I'm not 100% sure as I haven't yet compiled the docs to explicitly check

Comment threadpython/pyarrow/_compute.pyx Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why unique_ptr is used in some of the other classes, but it shouldn't be necessary. See MinMaxOptions for an example without.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PartitionNthOptions we need it because it doesn't have a null constructor so it can't be allocated on the stack. That's not the case for VarianceOptions, though, so I'll take a look

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewritten VarianceOptions without unique_ptr.

I think that with the other three options classes exposed in this PR (SetLookupOptions, PartitionNthOptions and StrptimeOptions) it's necessary to use unique_ptr or, alternatively, we would need to define default constructors

@kszucs

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-ubuntu-18.04-docs

@github-actions

Copy link
Copy Markdown

Revision: 73272bf38f2d8406b60a6ebaa20c5f4e58dff075

Submitted crossbow builds: ursa-labs/crossbow @ actions-615

TaskStatus
test-ubuntu-18.04-docsAzure

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Will merge after a couple changes. Thank you @arw2019 !

@pitrou

Copy link
Copy Markdown
Member

CI failures are unrelated.

@pitroupitrou closed this in ba7ee65Oct 8, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Thanks @pitrou@jorisvandenbossche for reviewing!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@arw2019@jorisvandenbossche@pitrou@kszucs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-9967: [Python] Add compute module docs + expose more option classes - #8145

Closed
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871
Closed

ARROW-9967: [Python] Add compute module docs + expose more option classes#8145
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871

Conversation

@arw2019

@arw2019arw2019 commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

#8163 exposes pyarrow.compute kernels and generates their docstrings. This PR adds documentation for the module in the User Guide and the Python API reference.

@arw2019
arw2019 marked this pull request as draft September 9, 2020 04:22
@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@arw2019arw2019 changed the title WIP: ARROW-7871: [Python] [DOC] Expose more compute kernels and add compute module docsARROW-9967: [Python] Add compute module docsSep 10, 2020
@github-actions

Copy link
Copy Markdown

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from edb57cc to 3f99ae8CompareSeptember 13, 2020 03:18
@arw2019
arw2019 marked this pull request as ready for review September 13, 2020 03:18
@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module docsARROW-9967: [Python] Add compute module documentationSep 13, 2020
Comment threaddocs/source/cpp/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that this was missing on the c++ page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you! However, can you leave the table alphabetically-sorted?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my bad, you already had it there. Reverted

@jorisvandenbossche

Copy link
Copy Markdown
Member

@arw2019 thanks a lot for working on this! (and sorry for the overlap with #8163)

Comment threaddocs/source/python/api/compute.rst Outdated
Comment threaddocs/source/python/api/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really a transform?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair. Would Structural Transforms be a good place for it?

Comment threaddocs/source/python/compute.rst Outdated
Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this array is not used? (maybe show pc.equals(a, b) ?)

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from e79559c to d117ed1CompareSeptember 15, 2020 04:10
Comment threadpython/pyarrow/compute.py Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche@pitrou

AFAIK we can't have methods named and and or in Python. For now I went with and_ and or_ here, for consistency with and_kleene and or_kleene, but for example numpy uses np.logical_and and np.logical_or

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a function named "and":

>>>frompyarrowimportcompute>>>getattr(compute, "and")
<functionpyarrow.compute.and(left, right, *, memory_pool=None)>

So you should just fix compute.py so that it's exported as and_.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed now

Comment threaddocs/source/python/api/compute.rst Outdated

@pitroupitrouSep 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this render the docstrings?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. It's what's done on the other pages:
https://github.com/apache/arrow/blob/master/docs/source/python/api/datatypes.rst

I'm having trouble building the docs locally (related to ARROW-10018 I think) so haven't been able to check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't render the docstring here, but it will create a separate page for each of those functions and link to that from this table.

(the separate pages might be a bit overkill since the docstrings here are often not that informative yet, but it's indeed how we do it for other functions as well)

@arw2019
arw2019force-pushed the ARROW-7871 branch 5 times, most recently from c2445c1 to c1c5846CompareSeptember 16, 2020 20:26
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Question: in 952649d I'm attempting to expose PartitionNthOptions but it errors at compilation. What is (are) my mistake(s)?

This is needed to make the partition_nth_indices Python wrapper usable.

@pitrou

Copy link
Copy Markdown
Member

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly. Also, it is better to use __cinit__ here so that the constructor never gets skipped, so for example (untested):

def__cinit__(self, int64_t pivot):
self.partition_nth_options.reset(new CPartitionNthOptions(pivot))

@arw2019

Copy link
Copy Markdown
ContributorAuthor

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly.

Thanks @pitrou! That was the problem

@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module documentationARROW-9967: [Python] Add compute module docs + expose more option classesSep 22, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

This is ready for re-review.

I believe that I've addressed the feedback from previous reviews. I've also now exposed all the option classes so that all the kernels listed in the docs are usable from Python.

Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche Will this work? I'm not 100% sure as I haven't yet compiled the docs to explicitly check

Comment threadpython/pyarrow/_compute.pyx Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why unique_ptr is used in some of the other classes, but it shouldn't be necessary. See MinMaxOptions for an example without.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PartitionNthOptions we need it because it doesn't have a null constructor so it can't be allocated on the stack. That's not the case for VarianceOptions, though, so I'll take a look

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewritten VarianceOptions without unique_ptr.

I think that with the other three options classes exposed in this PR (SetLookupOptions, PartitionNthOptions and StrptimeOptions) it's necessary to use unique_ptr or, alternatively, we would need to define default constructors

@kszucs

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-ubuntu-18.04-docs

@github-actions

Copy link
Copy Markdown

Revision: 73272bf38f2d8406b60a6ebaa20c5f4e58dff075

Submitted crossbow builds: ursa-labs/crossbow @ actions-615

TaskStatus
test-ubuntu-18.04-docsAzure

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Will merge after a couple changes. Thank you @arw2019 !

@pitrou

Copy link
Copy Markdown
Member

CI failures are unrelated.

@pitroupitrou closed this in ba7ee65Oct 8, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Thanks @pitrou@jorisvandenbossche for reviewing!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@arw2019@jorisvandenbossche@pitrou@kszucs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

ARROW-9967: [Python] Add compute module docs + expose more option classes - #8145

Closed
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871
Closed

ARROW-9967: [Python] Add compute module docs + expose more option classes#8145
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871

Conversation

@arw2019

@arw2019arw2019 commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

#8163 exposes pyarrow.compute kernels and generates their docstrings. This PR adds documentation for the module in the User Guide and the Python API reference.

@arw2019
arw2019 marked this pull request as draft September 9, 2020 04:22
@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@arw2019arw2019 changed the title WIP: ARROW-7871: [Python] [DOC] Expose more compute kernels and add compute module docsARROW-9967: [Python] Add compute module docsSep 10, 2020
@github-actions

Copy link
Copy Markdown

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from edb57cc to 3f99ae8CompareSeptember 13, 2020 03:18
@arw2019
arw2019 marked this pull request as ready for review September 13, 2020 03:18
@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module docsARROW-9967: [Python] Add compute module documentationSep 13, 2020
Comment threaddocs/source/cpp/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that this was missing on the c++ page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you! However, can you leave the table alphabetically-sorted?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my bad, you already had it there. Reverted

@jorisvandenbossche

Copy link
Copy Markdown
Member

@arw2019 thanks a lot for working on this! (and sorry for the overlap with #8163)

Comment threaddocs/source/python/api/compute.rst Outdated
Comment threaddocs/source/python/api/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really a transform?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair. Would Structural Transforms be a good place for it?

Comment threaddocs/source/python/compute.rst Outdated
Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this array is not used? (maybe show pc.equals(a, b) ?)

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from e79559c to d117ed1CompareSeptember 15, 2020 04:10
Comment threadpython/pyarrow/compute.py Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche@pitrou

AFAIK we can't have methods named and and or in Python. For now I went with and_ and or_ here, for consistency with and_kleene and or_kleene, but for example numpy uses np.logical_and and np.logical_or

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a function named "and":

>>>frompyarrowimportcompute>>>getattr(compute, "and")
<functionpyarrow.compute.and(left, right, *, memory_pool=None)>

So you should just fix compute.py so that it's exported as and_.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed now

Comment threaddocs/source/python/api/compute.rst Outdated

@pitroupitrouSep 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this render the docstrings?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. It's what's done on the other pages:
https://github.com/apache/arrow/blob/master/docs/source/python/api/datatypes.rst

I'm having trouble building the docs locally (related to ARROW-10018 I think) so haven't been able to check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't render the docstring here, but it will create a separate page for each of those functions and link to that from this table.

(the separate pages might be a bit overkill since the docstrings here are often not that informative yet, but it's indeed how we do it for other functions as well)

@arw2019
arw2019force-pushed the ARROW-7871 branch 5 times, most recently from c2445c1 to c1c5846CompareSeptember 16, 2020 20:26
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Question: in 952649d I'm attempting to expose PartitionNthOptions but it errors at compilation. What is (are) my mistake(s)?

This is needed to make the partition_nth_indices Python wrapper usable.

@pitrou

Copy link
Copy Markdown
Member

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly. Also, it is better to use __cinit__ here so that the constructor never gets skipped, so for example (untested):

def__cinit__(self, int64_t pivot):
self.partition_nth_options.reset(new CPartitionNthOptions(pivot))

@arw2019

Copy link
Copy Markdown
ContributorAuthor

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly.

Thanks @pitrou! That was the problem

@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module documentationARROW-9967: [Python] Add compute module docs + expose more option classesSep 22, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

This is ready for re-review.

I believe that I've addressed the feedback from previous reviews. I've also now exposed all the option classes so that all the kernels listed in the docs are usable from Python.

Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche Will this work? I'm not 100% sure as I haven't yet compiled the docs to explicitly check

Comment threadpython/pyarrow/_compute.pyx Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why unique_ptr is used in some of the other classes, but it shouldn't be necessary. See MinMaxOptions for an example without.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PartitionNthOptions we need it because it doesn't have a null constructor so it can't be allocated on the stack. That's not the case for VarianceOptions, though, so I'll take a look

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewritten VarianceOptions without unique_ptr.

I think that with the other three options classes exposed in this PR (SetLookupOptions, PartitionNthOptions and StrptimeOptions) it's necessary to use unique_ptr or, alternatively, we would need to define default constructors

@kszucs

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-ubuntu-18.04-docs

@github-actions

Copy link
Copy Markdown

Revision: 73272bf38f2d8406b60a6ebaa20c5f4e58dff075

Submitted crossbow builds: ursa-labs/crossbow @ actions-615

TaskStatus
test-ubuntu-18.04-docsAzure

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Will merge after a couple changes. Thank you @arw2019 !

@pitrou

Copy link
Copy Markdown
Member

CI failures are unrelated.

@pitroupitrou closed this in ba7ee65Oct 8, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Thanks @pitrou@jorisvandenbossche for reviewing!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@arw2019@jorisvandenbossche@pitrou@kszucs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-9967: [Python] Add compute module docs + expose more option classes - #8145

Closed
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871
Closed

ARROW-9967: [Python] Add compute module docs + expose more option classes#8145
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871

Conversation

@arw2019

@arw2019arw2019 commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

#8163 exposes pyarrow.compute kernels and generates their docstrings. This PR adds documentation for the module in the User Guide and the Python API reference.

@arw2019
arw2019 marked this pull request as draft September 9, 2020 04:22
@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@arw2019arw2019 changed the title WIP: ARROW-7871: [Python] [DOC] Expose more compute kernels and add compute module docsARROW-9967: [Python] Add compute module docsSep 10, 2020
@github-actions

Copy link
Copy Markdown

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from edb57cc to 3f99ae8CompareSeptember 13, 2020 03:18
@arw2019
arw2019 marked this pull request as ready for review September 13, 2020 03:18
@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module docsARROW-9967: [Python] Add compute module documentationSep 13, 2020
Comment threaddocs/source/cpp/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that this was missing on the c++ page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you! However, can you leave the table alphabetically-sorted?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my bad, you already had it there. Reverted

@jorisvandenbossche

Copy link
Copy Markdown
Member

@arw2019 thanks a lot for working on this! (and sorry for the overlap with #8163)

Comment threaddocs/source/python/api/compute.rst Outdated
Comment threaddocs/source/python/api/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really a transform?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair. Would Structural Transforms be a good place for it?

Comment threaddocs/source/python/compute.rst Outdated
Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this array is not used? (maybe show pc.equals(a, b) ?)

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from e79559c to d117ed1CompareSeptember 15, 2020 04:10
Comment threadpython/pyarrow/compute.py Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche@pitrou

AFAIK we can't have methods named and and or in Python. For now I went with and_ and or_ here, for consistency with and_kleene and or_kleene, but for example numpy uses np.logical_and and np.logical_or

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a function named "and":

>>>frompyarrowimportcompute>>>getattr(compute, "and")
<functionpyarrow.compute.and(left, right, *, memory_pool=None)>

So you should just fix compute.py so that it's exported as and_.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed now

Comment threaddocs/source/python/api/compute.rst Outdated

@pitroupitrouSep 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this render the docstrings?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. It's what's done on the other pages:
https://github.com/apache/arrow/blob/master/docs/source/python/api/datatypes.rst

I'm having trouble building the docs locally (related to ARROW-10018 I think) so haven't been able to check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't render the docstring here, but it will create a separate page for each of those functions and link to that from this table.

(the separate pages might be a bit overkill since the docstrings here are often not that informative yet, but it's indeed how we do it for other functions as well)

@arw2019
arw2019force-pushed the ARROW-7871 branch 5 times, most recently from c2445c1 to c1c5846CompareSeptember 16, 2020 20:26
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Question: in 952649d I'm attempting to expose PartitionNthOptions but it errors at compilation. What is (are) my mistake(s)?

This is needed to make the partition_nth_indices Python wrapper usable.

@pitrou

Copy link
Copy Markdown
Member

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly. Also, it is better to use __cinit__ here so that the constructor never gets skipped, so for example (untested):

def__cinit__(self, int64_t pivot):
self.partition_nth_options.reset(new CPartitionNthOptions(pivot))

@arw2019

Copy link
Copy Markdown
ContributorAuthor

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly.

Thanks @pitrou! That was the problem

@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module documentationARROW-9967: [Python] Add compute module docs + expose more option classesSep 22, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

This is ready for re-review.

I believe that I've addressed the feedback from previous reviews. I've also now exposed all the option classes so that all the kernels listed in the docs are usable from Python.

Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche Will this work? I'm not 100% sure as I haven't yet compiled the docs to explicitly check

Comment threadpython/pyarrow/_compute.pyx Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why unique_ptr is used in some of the other classes, but it shouldn't be necessary. See MinMaxOptions for an example without.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PartitionNthOptions we need it because it doesn't have a null constructor so it can't be allocated on the stack. That's not the case for VarianceOptions, though, so I'll take a look

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewritten VarianceOptions without unique_ptr.

I think that with the other three options classes exposed in this PR (SetLookupOptions, PartitionNthOptions and StrptimeOptions) it's necessary to use unique_ptr or, alternatively, we would need to define default constructors

@kszucs

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-ubuntu-18.04-docs

@github-actions

Copy link
Copy Markdown

Revision: 73272bf38f2d8406b60a6ebaa20c5f4e58dff075

Submitted crossbow builds: ursa-labs/crossbow @ actions-615

TaskStatus
test-ubuntu-18.04-docsAzure

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Will merge after a couple changes. Thank you @arw2019 !

@pitrou

Copy link
Copy Markdown
Member

CI failures are unrelated.

@pitroupitrou closed this in ba7ee65Oct 8, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Thanks @pitrou@jorisvandenbossche for reviewing!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@arw2019@jorisvandenbossche@pitrou@kszucs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-9967: [Python] Add compute module docs + expose more option classes - #8145

Closed
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871
Closed

ARROW-9967: [Python] Add compute module docs + expose more option classes#8145
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871

Conversation

@arw2019

@arw2019arw2019 commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

#8163 exposes pyarrow.compute kernels and generates their docstrings. This PR adds documentation for the module in the User Guide and the Python API reference.

@arw2019
arw2019 marked this pull request as draft September 9, 2020 04:22
@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@arw2019arw2019 changed the title WIP: ARROW-7871: [Python] [DOC] Expose more compute kernels and add compute module docsARROW-9967: [Python] Add compute module docsSep 10, 2020
@github-actions

Copy link
Copy Markdown

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from edb57cc to 3f99ae8CompareSeptember 13, 2020 03:18
@arw2019
arw2019 marked this pull request as ready for review September 13, 2020 03:18
@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module docsARROW-9967: [Python] Add compute module documentationSep 13, 2020
Comment threaddocs/source/cpp/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that this was missing on the c++ page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you! However, can you leave the table alphabetically-sorted?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my bad, you already had it there. Reverted

@jorisvandenbossche

Copy link
Copy Markdown
Member

@arw2019 thanks a lot for working on this! (and sorry for the overlap with #8163)

Comment threaddocs/source/python/api/compute.rst Outdated
Comment threaddocs/source/python/api/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really a transform?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair. Would Structural Transforms be a good place for it?

Comment threaddocs/source/python/compute.rst Outdated
Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this array is not used? (maybe show pc.equals(a, b) ?)

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from e79559c to d117ed1CompareSeptember 15, 2020 04:10
Comment threadpython/pyarrow/compute.py Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche@pitrou

AFAIK we can't have methods named and and or in Python. For now I went with and_ and or_ here, for consistency with and_kleene and or_kleene, but for example numpy uses np.logical_and and np.logical_or

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a function named "and":

>>>frompyarrowimportcompute>>>getattr(compute, "and")
<functionpyarrow.compute.and(left, right, *, memory_pool=None)>

So you should just fix compute.py so that it's exported as and_.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed now

Comment threaddocs/source/python/api/compute.rst Outdated

@pitroupitrouSep 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this render the docstrings?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. It's what's done on the other pages:
https://github.com/apache/arrow/blob/master/docs/source/python/api/datatypes.rst

I'm having trouble building the docs locally (related to ARROW-10018 I think) so haven't been able to check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't render the docstring here, but it will create a separate page for each of those functions and link to that from this table.

(the separate pages might be a bit overkill since the docstrings here are often not that informative yet, but it's indeed how we do it for other functions as well)

@arw2019
arw2019force-pushed the ARROW-7871 branch 5 times, most recently from c2445c1 to c1c5846CompareSeptember 16, 2020 20:26
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Question: in 952649d I'm attempting to expose PartitionNthOptions but it errors at compilation. What is (are) my mistake(s)?

This is needed to make the partition_nth_indices Python wrapper usable.

@pitrou

Copy link
Copy Markdown
Member

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly. Also, it is better to use __cinit__ here so that the constructor never gets skipped, so for example (untested):

def__cinit__(self, int64_t pivot):
self.partition_nth_options.reset(new CPartitionNthOptions(pivot))

@arw2019

Copy link
Copy Markdown
ContributorAuthor

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly.

Thanks @pitrou! That was the problem

@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module documentationARROW-9967: [Python] Add compute module docs + expose more option classesSep 22, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

This is ready for re-review.

I believe that I've addressed the feedback from previous reviews. I've also now exposed all the option classes so that all the kernels listed in the docs are usable from Python.

Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche Will this work? I'm not 100% sure as I haven't yet compiled the docs to explicitly check

Comment threadpython/pyarrow/_compute.pyx Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why unique_ptr is used in some of the other classes, but it shouldn't be necessary. See MinMaxOptions for an example without.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PartitionNthOptions we need it because it doesn't have a null constructor so it can't be allocated on the stack. That's not the case for VarianceOptions, though, so I'll take a look

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewritten VarianceOptions without unique_ptr.

I think that with the other three options classes exposed in this PR (SetLookupOptions, PartitionNthOptions and StrptimeOptions) it's necessary to use unique_ptr or, alternatively, we would need to define default constructors

@kszucs

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-ubuntu-18.04-docs

@github-actions

Copy link
Copy Markdown

Revision: 73272bf38f2d8406b60a6ebaa20c5f4e58dff075

Submitted crossbow builds: ursa-labs/crossbow @ actions-615

TaskStatus
test-ubuntu-18.04-docsAzure

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Will merge after a couple changes. Thank you @arw2019 !

@pitrou

Copy link
Copy Markdown
Member

CI failures are unrelated.

@pitroupitrou closed this in ba7ee65Oct 8, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Thanks @pitrou@jorisvandenbossche for reviewing!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@arw2019@jorisvandenbossche@pitrou@kszucs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

ARROW-9967: [Python] Add compute module docs + expose more option classes - #8145

Closed
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871
Closed

ARROW-9967: [Python] Add compute module docs + expose more option classes#8145
arw2019 wants to merge 18 commits into
apache:masterfrom
arw2019:ARROW-7871

Conversation

@arw2019

@arw2019arw2019 commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

#8163 exposes pyarrow.compute kernels and generates their docstrings. This PR adds documentation for the module in the User Guide and the Python API reference.

@arw2019
arw2019 marked this pull request as draft September 9, 2020 04:22
@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@arw2019arw2019 changed the title WIP: ARROW-7871: [Python] [DOC] Expose more compute kernels and add compute module docsARROW-9967: [Python] Add compute module docsSep 10, 2020
@github-actions

Copy link
Copy Markdown

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from edb57cc to 3f99ae8CompareSeptember 13, 2020 03:18
@arw2019
arw2019 marked this pull request as ready for review September 13, 2020 03:18
@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module docsARROW-9967: [Python] Add compute module documentationSep 13, 2020
Comment threaddocs/source/cpp/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that this was missing on the c++ page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you! However, can you leave the table alphabetically-sorted?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my bad, you already had it there. Reverted

@jorisvandenbossche

Copy link
Copy Markdown
Member

@arw2019 thanks a lot for working on this! (and sorry for the overlap with #8163)

Comment threaddocs/source/python/api/compute.rst Outdated
Comment threaddocs/source/python/api/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really a transform?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair. Would Structural Transforms be a good place for it?

Comment threaddocs/source/python/compute.rst Outdated
Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this array is not used? (maybe show pc.equals(a, b) ?)

@arw2019
arw2019force-pushed the ARROW-7871 branch 2 times, most recently from e79559c to d117ed1CompareSeptember 15, 2020 04:10
Comment threadpython/pyarrow/compute.py Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche@pitrou

AFAIK we can't have methods named and and or in Python. For now I went with and_ and or_ here, for consistency with and_kleene and or_kleene, but for example numpy uses np.logical_and and np.logical_or

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a function named "and":

>>>frompyarrowimportcompute>>>getattr(compute, "and")
<functionpyarrow.compute.and(left, right, *, memory_pool=None)>

So you should just fix compute.py so that it's exported as and_.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed now

Comment threaddocs/source/python/api/compute.rst Outdated

@pitroupitrouSep 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this render the docstrings?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. It's what's done on the other pages:
https://github.com/apache/arrow/blob/master/docs/source/python/api/datatypes.rst

I'm having trouble building the docs locally (related to ARROW-10018 I think) so haven't been able to check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't render the docstring here, but it will create a separate page for each of those functions and link to that from this table.

(the separate pages might be a bit overkill since the docstrings here are often not that informative yet, but it's indeed how we do it for other functions as well)

@arw2019
arw2019force-pushed the ARROW-7871 branch 5 times, most recently from c2445c1 to c1c5846CompareSeptember 16, 2020 20:26
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Question: in 952649d I'm attempting to expose PartitionNthOptions but it errors at compilation. What is (are) my mistake(s)?

This is needed to make the partition_nth_indices Python wrapper usable.

@pitrou

Copy link
Copy Markdown
Member

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly. Also, it is better to use __cinit__ here so that the constructor never gets skipped, so for example (untested):

def__cinit__(self, int64_t pivot):
self.partition_nth_options.reset(new CPartitionNthOptions(pivot))

@arw2019

Copy link
Copy Markdown
ContributorAuthor

Cython generally doesn't convert implicitly between arbitrary Python objects and C/C++ types, you have to type your code explicitly.

Thanks @pitrou! That was the problem

@arw2019arw2019 changed the title ARROW-9967: [Python] Add compute module documentationARROW-9967: [Python] Add compute module docs + expose more option classesSep 22, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

This is ready for re-review.

I believe that I've addressed the feedback from previous reviews. I've also now exposed all the option classes so that all the kernels listed in the docs are usable from Python.

Comment threaddocs/source/python/compute.rst Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche Will this work? I'm not 100% sure as I haven't yet compiled the docs to explicitly check

Comment threadpython/pyarrow/_compute.pyx Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why unique_ptr is used in some of the other classes, but it shouldn't be necessary. See MinMaxOptions for an example without.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PartitionNthOptions we need it because it doesn't have a null constructor so it can't be allocated on the stack. That's not the case for VarianceOptions, though, so I'll take a look

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewritten VarianceOptions without unique_ptr.

I think that with the other three options classes exposed in this PR (SetLookupOptions, PartitionNthOptions and StrptimeOptions) it's necessary to use unique_ptr or, alternatively, we would need to define default constructors

@kszucs

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-ubuntu-18.04-docs

@github-actions

Copy link
Copy Markdown

Revision: 73272bf38f2d8406b60a6ebaa20c5f4e58dff075

Submitted crossbow builds: ursa-labs/crossbow @ actions-615

TaskStatus
test-ubuntu-18.04-docsAzure

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Will merge after a couple changes. Thank you @arw2019 !

@pitrou

Copy link
Copy Markdown
Member

CI failures are unrelated.

@pitroupitrou closed this in ba7ee65Oct 8, 2020
@arw2019

Copy link
Copy Markdown
ContributorAuthor

Thanks @pitrou@jorisvandenbossche for reviewing!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@arw2019@jorisvandenbossche@pitrou@kszucs