Skip to content
This repository was archived by the owner on Feb 2, 2024. It is now read-only.

Conversation

@1e-to
Copy link
Contributor

@1e-to 1e-to commented Feb 14, 2020

image

@pep8speaks
Copy link

pep8speaks commented Feb 14, 2020

Hello @1e-to! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-19 07:25:08 UTC

return nanprod_impl


def get_pool_size():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this file to something like prange_utils.py

if pool_size == 0:
pool_size = get_pool_size()

chunk_size = size//pool_size + 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
chunk_size = size//pool_size + 1
chunk_size = (size - 1)//pool_size + 1

this is the correct formula

if pool_size == 0:
pool_size = get_pool_size()

chunk_size = size//pool_size + 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

(size - 1)//pool_size + 1

Copy link
Collaborator

@AlexanderKalistratov AlexanderKalistratov left a comment

Choose a reason for hiding this comment

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

👍

result_index = numpy.empty(shape=length, dtype=dtype_idx)
for i in prange(len(chunks)):
chunk = chunks[i]
if i == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

new_start = sum(arr_len[0:i])
new_stop = new_start + arr_len[i]

for j in range(chunk.start, chunk.stop):
if new_start < new_stop:
if not isnan(arr[j]):
result_data[new_start] = arr[j]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is better to introduce new variable. Something like current_pos. Like this:

current_pos = new_start
for j in range(chunk.start, chunk.stop):
    if current_pos < new_stop:
        if not isnan(arr[j]):
            result_data[current_pos] = arr[j]
            result_index[current_pos] = idx[j]
            current_pos += 1

It is confusing that you are always writing to new_start

new_stop = new_start + arr_len[i]

for j in range(chunk.start, chunk.stop):
if new_start < new_stop:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if you actually need this condition

@AlexanderKalistratov
Copy link
Collaborator

conflicts

@AlexanderKalistratov
Copy link
Collaborator

Could you please remeasure performance?

@1e-to
Copy link
Contributor Author

1e-to commented Feb 19, 2020

ase remeasure performance?

It's still like in a picture one:
image

@AlexanderKalistratov AlexanderKalistratov merged commit 6b53d4c into IntelPython:master Feb 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants