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 missing shape parameter to numpy methods #1450

Merged
merged 9 commits into from
Mar 9, 2022

Conversation

mbyrnepr2
Copy link
Member

@mbyrnepr2 mbyrnepr2 commented Mar 7, 2022

Add the missing shape parameter to the brain for:

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

Closes with relevant test pylint-dev/pylint#5871

@mbyrnepr2 mbyrnepr2 changed the title Fix false positive for shape parameter of numpy.zeros_like me… False positive for shape parameter of numpy.zeros_like Mar 7, 2022
@DanielNoord
Copy link
Collaborator

@mbyrnepr2 👍
Could you add a test for this?

@mbyrnepr2
Copy link
Member Author

@DanielNoord sure thing. I am less familiar with astroid code so I will take a look at the test structure for this hopefully later today. If you can give me any pointers I would appreciate.

@DanielNoord
Copy link
Collaborator

@DanielNoord sure thing. I am less familiar with astroid code so I will take a look at the test structure for this hopefully later today. If you can give me any pointers I would appreciate.

I'm on mobile so I can't look up code easily right now.
Searching for numpy or np should probably show the other tests for this brain. Often we simply test that the thing we're inferring is correct.
Note: some tests use next(node.inferred()). We're trying to avoid this as this doesn't show whether there is ambiguity in the inference (which we often don't want). So it's often better to cast inferred() to a list and check that the length of that list is 1.

@mbyrnepr2
Copy link
Member Author

@DanielNoord sure thing. I am less familiar with astroid code so I will take a look at the test structure for this hopefully later today. If you can give me any pointers I would appreciate.

I'm on mobile so I can't look up code easily right now. Searching for numpy or np should probably show the other tests for this brain. Often we simply test that the thing we're inferring is correct. Note: some tests use next(node.inferred()). We're trying to avoid this as this doesn't show whether there is ambiguity in the inference (which we often don't want). So it's often better to cast inferred() to a list and check that the length of that list is 1.

Great that's good to know, will try it out later @DanielNoord

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.11.0 milestone Mar 8, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you for your First astroid contribution @mbyrnepr2 ;)

@cdce8p
Copy link
Member

cdce8p commented Mar 8, 2022

I am less familiar with astroid code so I will take a look at the test structure for this hopefully later today. If you can give me any pointers I would appreciate.

Adding another item here should work.
https://github.com/PyCQA/astroid/blob/29dcc35ac14ac21fe97502ac890a1faac936054e/tests/unittest_brain_numpy_core_numeric.py#L28-L29

@mbyrnepr2
Copy link
Member Author

I am less familiar with astroid code so I will take a look at the test structure for this hopefully later today. If you can give me any pointers I would appreciate.

Adding another item here should work.

https://github.com/PyCQA/astroid/blob/29dcc35ac14ac21fe97502ac890a1faac936054e/tests/unittest_brain_numpy_core_numeric.py#L28-L29

Thanks @cdce8p! I was looking at this one earlier. It seems that the tests will pass with or without adding the parameter to this line; but that seems to be the structure for much of the numpy tests that I've seen.
So, for example, I have it set to ("zeros_like", "[1, 2]", "None", "K", "True", "None"), in my env. but the results are the same if I leave these optional parameters out also.

@mbyrnepr2
Copy link
Member Author

Thank you for your First astroid contribution @mbyrnepr2 ;)

Thanks for the warm welcome, nice to be here!

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.

Change itself looks good to me!

tests/unittest_brain_numpy_core_numeric.py Show resolved Hide resolved
tests/unittest_brain_numpy_core_numeric.py Outdated Show resolved Hide resolved
- Fixup imports
- More accurate unittest naming
- Use typehints
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Added some last changes in aeb1bf8. Looks good now!

Although not strictly necessary as astroid can't infer numpy natively anyway, I added the >1.17.0 requirement. According to the docs, that's where shape was added.
https://numpy.org/doc/stable/reference/generated/numpy.zeros_like.html

@mbyrnepr2
Copy link
Member Author

Added some last changes in aeb1bf8. Looks good now!

Although not strictly necessary as astroid can't infer numpy natively anyway, I added the >1.17.0 requirement. According to the docs, that's where shape was added. https://numpy.org/doc/stable/reference/generated/numpy.zeros_like.html

Thank you for this @cdce8p!

@cdce8p cdce8p changed the title False positive for shape parameter of numpy.zeros_like Add missing shape parameter to numpy methods Mar 9, 2022
@cdce8p cdce8p merged commit 6d81868 into pylint-dev:main Mar 9, 2022
@cdce8p
Copy link
Member

cdce8p commented Mar 9, 2022

And it's merged. Congrats on your first astroid contribution @mbyrnepr2 🚀

@mbyrnepr2 mbyrnepr2 deleted the pylint_issue_5871 branch March 10, 2022 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Brain 🧠 Needs a brain tip Bug 🪳
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants