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

remove multiple exit from prange #1901

Merged
merged 8 commits into from
Oct 16, 2024
Merged

remove multiple exit from prange #1901

merged 8 commits into from
Oct 16, 2024

Conversation

paskino
Copy link
Contributor

@paskino paskino commented Aug 22, 2024

Description

Changes

  • It uses an accumulator array with each array adding to a location within it depending on the thread_id
  • It returns the sum of the accumulator array
  • It skips the calculation of the KL divergence if any thread evaluates to np.inf using a single int. Numba might be doing some atomic checks for writing to that int, but race conditions may happen very seldom (after discussion with @gfardell )

Testing you performed

Please add any demo scripts to https://github.com/TomographicImaging/CIL-Demos/tree/main/misc

Related issues/links

Checklist

  • I have performed a self-review of my code
  • I have added docstrings in line with the guidance in the developer guide
  • I have updated the relevant documentation
  • I have implemented unit tests that cover any new or modified functionality
  • CHANGELOG.md has been updated with any functionality change
  • Request review from all relevant developers
  • Change pull request label to 'Waiting for review'

Contribution Notes

Please read and adhere to the developer guide and local patterns and conventions.

  • The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in CIL (the Work) under the terms and conditions of the Apache-2.0 License
  • I confirm that the contribution does not violate any intellectual property rights of third parties

closes #1900 

Signed-off-by: Edoardo Pasca <[email protected]>
@paskino paskino self-assigned this Aug 22, 2024
Copy link
Member

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

can you break in a prange to stop all other threads?

@paskino
Copy link
Contributor Author

paskino commented Aug 22, 2024

I couldn't find any documentation about breaking a prange, did you?

@paskino paskino closed this Aug 22, 2024
@casperdcl
Copy link
Member

No, which is why I asked...

@paskino paskino reopened this Sep 5, 2024
@paskino paskino modified the milestone: v24.2.0 Sep 24, 2024
@paskino
Copy link
Contributor Author

paskino commented Sep 24, 2024

The following code seems to suggest that break in a prange creates a multiple exit scenario:

import numba as nb
import numpy as np

@nb.njit(parallel=True) 
def break_prange(x, n): 
    accumulator = np.zeros(nb.get_num_threads(), dtype=np.float64) 
    for i in nb.prange(x.size):
        if i > n:
            break
        else:
            accumulator[i] = i
    return accumulator


x = np.arange(nb.get_num_threads() + 10)
x = np.asarray(x, dtype=np.float32)

y = break_prange(x,10) 

print (y)

it returns

/opt/conda/lib/python3.10/site-packages/numba/parfors/parfor.py:2395: NumbaPerformanceWarning: 
prange or pndindex loop will not be executed in parallel due to there being more than one entry to or exit from the loop (e.g., an assertion).

File "../../../../../../tmp/ipykernel_402/3173854079.py", line 7:
<source missing, REPL/exec in use?>

  warnings.warn(
[ 0.  1.  2.  3.  4.  5.  6.  7.  8.  9. 10.  0.  0.  0.  0.  0.  0.  0.
  0.  0.  0.  0.  0.  0.  0.  0.  0.  0.]

Swapping break with pass still works and raises no warning.

Signed-off-by: Edoardo Pasca <[email protected]>
@paskino paskino marked this pull request as ready for review September 24, 2024 14:03
@paskino paskino requested a review from gfardell October 1, 2024 10:24
Copy link
Contributor

@hrobarts hrobarts left a comment

Choose a reason for hiding this comment

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

Hi Edo, this looks sensible to me.

Copy link
Member

@gfardell gfardell left a comment

Choose a reason for hiding this comment

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

The change log needs to move to 24.3.0 rather than the released 24.2. Otherwise it looks good to me.

@paskino paskino merged commit 6485259 into master Oct 16, 2024
11 checks passed
@paskino paskino deleted the kl-multiple-exits branch October 16, 2024 12:22
Sign up for free to 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.

NumbaPerformanceWarning on KL
4 participants