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

Add brain for numpy core module einsumfunc. #1656

Merged
merged 14 commits into from Jul 6, 2022

Conversation

mbyrnepr2
Copy link
Member

Refs pylint-dev/pylint#5821

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Related Issue

@coveralls
Copy link

coveralls commented Jun 23, 2022

Pull Request Test Coverage Report for Build 2621848802

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 92.124%

Totals Coverage Status
Change from base Build 2613849741: 0.006%
Covered Lines: 9439
Relevant Lines: 10246

💛 - Coveralls

@mbyrnepr2 mbyrnepr2 marked this pull request as ready for review June 23, 2022 07:54
@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Jun 23, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone Jun 23, 2022
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Not sure why this landed in PR limbo, but I thought I'd start with a review.

More general question: why are we putting al numpy brains into separate files? Seems like a bit of a waste, right? Or am I missing something?

tests/unittest_brain_numpy_core_einsumfunc.py Outdated Show resolved Hide resolved
tests/unittest_brain_numpy_core_einsumfunc.py Outdated Show resolved Hide resolved
ChangeLog Outdated Show resolved Hide resolved
astroid/brain/brain_numpy_core_einsumfunc.py Outdated Show resolved Hide resolved
mbyrnepr2 and others added 4 commits July 2, 2022 13:51
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
@mbyrnepr2
Copy link
Member Author

mbyrnepr2 commented Jul 2, 2022

Not sure why this landed in PR limbo, but I thought I'd start with a review.

More general question: why are we putting al numpy brains into separate files? Seems like a bit of a waste, right? Or am I missing something?

Thanks!

I don't know the story behind this but to my mind it seems more maintainable to have them as separate modules, especially if they increase in size over time. Perhaps we could create a separate sub-package astroid/brain/numpy/ to collect them in one place?

Or perhaps your point is more related to performance @DanielNoord?

@DanielNoord
Copy link
Collaborator

I don't know the story behind this but to my mind it seems more maintainable to have them as separate modules, especially if they increase in size over time.

Yeah I think a refactor might be beneficial as I'm not sure this is actually more maintainable. For example, the brain you added could have been added to https://github.com/PyCQA/astroid/blob/main/astroid/brain/brain_numpy_core_fromnumeric.py.
That would have removed 17 unnecessary lines from this diff in the brain file alone.

Same probably goes for test files.

@mbyrnepr2
Copy link
Member Author

mbyrnepr2 commented Jul 2, 2022

Hi @DanielNoord I think you have good ideas. As a way forward may I suggest you create a new issue to capture the ideas behind your refactoring suggestion? I'm sure there are knowledgeable folks who are aware of the reasoning behind the existing structure who can provide input & help move that forward.

The current issue can move forward independently & according to the existing structure; if the refactoring issue comes to fruition then it wouldn't be much extra work to incorporate einsumfunc brain into the new system.

What do you think?

@DanielNoord
Copy link
Collaborator

I'm fine with creating a new issue, but I'm not sure if there is anybody around anymore that knows the original rationale.

Why don't approach this from what we would want ourselves. If you think keeping these separate is better that's fine with me, but if you think it's better to merge them then let's do so now!

@mbyrnepr2
Copy link
Member Author

Mainly I think for now I'd like to follow the existing structure in order to stick to the scope of fixing the Pylint issue; hopefully I have time to finish that much this week.
Regarding the refactor itself - I'm in two minds because I think the structure now is helpful with the 1:1 of brain to numpy module mapping; but perhaps I am missing something.

tests/unittest_brain_numpy_core_einsumfunc.py Outdated Show resolved Hide resolved
tests/unittest_brain_numpy_core_einsumfunc.py Outdated Show resolved Hide resolved
tests/unittest_brain_numpy_core_einsumfunc.py Outdated Show resolved Hide resolved
mbyrnepr2 and others added 3 commits July 6, 2022 11:18
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
@DanielNoord
Copy link
Collaborator

Pushed some final changes. Thanks @mbyrnepr2 😄

@DanielNoord DanielNoord merged commit 2a16144 into pylint-dev:main Jul 6, 2022
@mbyrnepr2 mbyrnepr2 deleted the pylint_issue_5821 branch July 6, 2022 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants