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

Fix overload_method problem with stararg #4978

Merged
merged 6 commits into from Dec 18, 2019
Merged

Fix overload_method problem with stararg #4978

merged 6 commits into from Dec 18, 2019

Conversation

sklam
Copy link
Member

@sklam sklam commented Dec 17, 2019 •
edited

Fix #4944

  • Fixes creating extra tuple that contains the *args tuple
  • Adds StarArgTuple to tell from tuple in *args versus tuple created by it.

@sklam sklam added this to the Numba 0.47 RC milestone Dec 17, 2019
@seibert
Copy link
Contributor

seibert commented Dec 18, 2019

internal CI: numba_smoketest_cpu_5

stuartarchibald
stuartarchibald reviewed Dec 18, 2019
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 patch, I've tried it locally and it seems good. Whilst this is small it must have taken ages to work this out, good job! One minor comment to resolve else good to go.

numba/types/containers.py
Comment on lines +303 to +317
class StarArgTuple(Tuple):
"""To distinguish from Tuple() used as argument to a `*args`.
"""
def __new__(cls, types):
_HeterogeneousTuple.is_types_iterable(types)

if types and all(t == types[0] for t in types[1:]):
return StarArgUniTuple(dtype=types[0], count=len(types))
else:
return object.__new__(StarArgTuple)


class StarArgUniTuple(UniTuple):
"""To distinguish from UniTuple() used as argument to a `*args`.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these get a custom name written to self.name so as to distinguish the type in traceback/debug?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch. yes it needs new name

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 90956ff

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Dec 18, 2019
stuartarchibald
stuartarchibald approved these changes Dec 18, 2019
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 fix up.

@stuartarchibald stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on author Waiting for author to respond to review labels Dec 18, 2019
@stuartarchibald
Copy link
Contributor

Think this is waiting on CI now.

@stuartarchibald stuartarchibald added the Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm label Dec 18, 2019
@seibert
Copy link
Contributor

seibert commented Dec 18, 2019

internal CI passed

@seibert seibert added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on CI Review etc done, waiting for CI to finish Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm labels Dec 18, 2019
@seibert seibert merged commit 63fbcfd into numba:master Dec 18, 2019
@sklam sklam deleted the fix/iss4944 branch December 18, 2019 19:52
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

Assignees
No one assigned
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Milestone
Numba 0.47 RC
Development

Successfully merging this pull request may close these issues.

master: @overload_method issues with *args
3 participants
@sklam @seibert @stuartarchibald