Skip to content

Refactoring ParforDiagnostics - #6545

Merged
sklam merged 2 commits into
numba:masterfrom
IntelPython:refactoring_parfor
Dec 17, 2020
Merged

Refactoring ParforDiagnostics#6545
sklam merged 2 commits into
numba:masterfrom
IntelPython:refactoring_parfor

Conversation

@1e-to

@1e-to 1e-to commented Dec 4, 2020

Copy link
Copy Markdown
Contributor

We need to be able to get "all_lines" to reuse it in our ExtendedParforDiagnostics to avoid code duplication.
Also now "dump" function is too complicated so this refactoring was made.

@1e-to 1e-to mentioned this pull request Dec 4, 2020
Closed
41 of 50 tasks
@stuartarchibald stuartarchibald added this to the PR Backlog milestone Dec 4, 2020
@1e-to
1e-to force-pushed the refactoring_parfor branch from 0519e3a to e0c98f5 Compare December 7, 2020 11:37
@1e-to
1e-to force-pushed the refactoring_parfor branch from e0c98f5 to c391c95 Compare December 7, 2020 12:10
@1e-to 1e-to mentioned this pull request Dec 7, 2020
Open
@stuartarchibald

stuartarchibald commented Dec 7, 2020

Copy link
Copy Markdown
Contributor

@1e-to thanks for the patch. Before review, just want to check that the intention here is largely code motion with view of putting the diagnostics stuff all in the same place? There should be no functional change?

@1e-to

1e-to commented Dec 7, 2020

Copy link
Copy Markdown
Contributor Author

@1e-to thanks for the patch. Before review, just want to check that the intention here is largely code motion with view of putting the diagnostics stuff all in the same place? There should be no functional change?

Yes, some functionality has been moved from the dump to separate functions so that we can reuse it.

@1e-to 1e-to mentioned this pull request Dec 8, 2020
Merged
@stuartarchibald stuartarchibald added the Effort - medium Medium size effort needed label Dec 11, 2020
stuartarchibald
stuartarchibald approved these changes Dec 14, 2020
View reviewed changes

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch. The diagnostic reporting isn't heavily tested in the Numba test suite, however this patch is pretty much just code motion with a small amount of clean up of dead code so I think the risk is minimal. The refactoring presented is a good improvement over having the diagnostics inlined. I've checked this locally and the reports seem identical. Happy to merge as-is. Thanks again!

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 3 - Ready for Review labels Dec 14, 2020
@sklam
sklam merged commit a21e5ba into numba:master Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartarchibald stuartarchibald stuartarchibald approved these changes
@DrTodd13 DrTodd13 Awaiting requested review from DrTodd13
@esc esc Awaiting requested review from esc
@sklam sklam Awaiting requested review from sklam

Assignees

No one assigned

Labels

5 - Ready to merge Review and testing done, is ready to merge Effort - medium Medium size effort needed

Projects

None yet

Milestone

PR Backlog

Development

Successfully merging this pull request may close these issues.

3 participants

@1e-to @stuartarchibald @sklam