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.

Sign up for GitHub

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

Jump to bottom

Add del's to reductions. #5047

Closed
wants to merge 6 commits into from
Closed

Add del's to reductions. #5047

wants to merge 6 commits into from

Conversation

DrTodd13
Copy link
Collaborator

@DrTodd13 DrTodd13 commented Jan 7, 2020 •
edited

Resolves #5003

@stuartarchibald stuartarchibald added this to the Numba 0.49 RC milestone Jan 16, 2020
stuartarchibald
stuartarchibald reviewed Jan 16, 2020
View reviewed changes
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This seems like it'd help with leaks (just by what's being done), can the MemoryLeakMixin be used in a test to check for the leak being fixed? Would that work? I'm basically wondering how we programmatically check this fix. Thanks again!

numba/npyufunc/parfor.py Outdated
Comment on lines 1555 to 1557
# for v in redarrdict.values():
# lowerer.lower_inst(ir.Del(v.name, loc=loc))

Copy link
Contributor

Choose a reason for hiding this comment

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

dead code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the code that made me want to look at the PR again. Want to make sure the leak appears to be gone. I also had the same question about how to test this and I don't know the answer. @stuartarchibald

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, thanks. I'll put working out how to test this on my TODOs, I'm sure we can come up with something, even if we have to write something new it'd probably prove quite useful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed those commented lines.

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jan 16, 2020
@DrTodd13
Copy link
Collaborator Author

@stuartarchibald What are you waiting for me on this PR?

@stuartarchibald
Copy link
Contributor

@stuartarchibald What are you waiting for me on this PR?

I think this is just waiting for a test, which either you are I need to work out how to do :) ?

@github-actions
Copy link

This pull request is marked as stale as it has had no activity in the past 3 months. Please respond to this comment if you're still interested in working on this. Many thanks!

@github-actions github-actions bot added the stale Marker label for stale issues. label Apr 26, 2023
@github-actions github-actions bot added the abandoned - stale PRs automatically closed due to stale status label May 4, 2023
@github-actions github-actions bot closed this May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartarchibald stuartarchibald stuartarchibald left review comments

Assignees
No one assigned
Labels
4 - Waiting on author Waiting for author to respond to review abandoned - stale PRs automatically closed due to stale status stale Marker label for stale issues.
Projects
None yet
Milestone
PR Backlog
Development

Successfully merging this pull request may close these issues.

Memory leak when allocating arrays in function in parallel loop
2 participants
@DrTodd13 @stuartarchibald