Skip to content

Support for dict comprehension - #6736

Merged
sklam merged 2 commits into
numba:masterfrom
stuartarchibald:fix/5135
Feb 23, 2021
Merged

Support for dict comprehension#6736
sklam merged 2 commits into
numba:masterfrom
stuartarchibald:fix/5135

Conversation

@stuartarchibald

@stuartarchibald stuartarchibald commented Feb 19, 2021

Copy link
Copy Markdown
Contributor

As title.

Closes: #5135

@stuartarchibald stuartarchibald added this to the PR Backlog milestone Feb 19, 2021
@stuartarchibald stuartarchibald mentioned this pull request Feb 19, 2021
Closed
@stuartarchibald stuartarchibald added the Effort - medium Medium size effort needed label Feb 19, 2021
esc
esc requested changes Feb 23, 2021
View reviewed changes

@esc esc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One suggestion to fix doc formatting and a question about test coverage.

Comment thread docs/source/reference/pysupported.rst Outdated
Comment on lines +808 to +811
In [2]: @njit
...: def foo(n):
...: return {i: i**2 for i in range(n)}
...:

@esc esc Feb 23, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
In [2]: @njit
...: def foo(n):
...: return {i: i**2 for i in range(n)}
...:
In [2]: @njit
...: def foo(n):
...: return {i: i**2 for i in range(n)}
...:

@stuartarchibald stuartarchibald Feb 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

well spotted, colons are lined up in 7145e6c

@esc esc Feb 23, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

excellent, ready to smoketest then?

@stuartarchibald stuartarchibald Feb 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Think so!

Comment thread numba/core/dataflow.py
info.append(inst, items=items[::-1], size=count, res=dct)
info.push(dct)

def op_MAP_ADD(self, info, inst):

@esc esc Feb 23, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tried to check if this code was ever exercised, by using the patch:

Encountered the use of a type that is scheduled for deprecation: type 'reflected list' found for argument 'vals' of function 'TestDictObject.test_dict_values.<locals>.diff --git i/numba/core/byteflow.py w/numba/core/byteflow.py
index 5c612f3a08..bf7f48cf84 100644
--- i/numba/core/byteflow.py
+++ w/numba/core/byteflow.py
@@ -920,6 +920,8 @@ class TraceRunner(object):
         # NOTE: https://docs.python.org/3/library/dis.html#opcode-MAP_ADD
         # Python >= 3.8: TOS and TOS1 are value and key respectively
         # Python < 3.8: TOS and TOS1 are key and value respectively
+        import ipdb
+        ipdb.set_trace()
         TOS = state.pop()
         TOS1 = state.pop()
         key, value = (TOS, TOS1) if PYVERSION < (3, 8) else (TOS1, TOS)
diff --git i/numba/core/dataflow.py w/numba/core/dataflow.py
index c351086a50..646103e682 100644
--- i/numba/core/dataflow.py
+++ w/numba/core/dataflow.py
@@ -203,6 +203,8 @@ class DataFlowAnalysis(object):
         info.push(dct)

     def op_MAP_ADD(self, info, inst):
+        import ipdb
+        ipdb.set_trace()
         key = info.pop()
         value = info.pop()
         index = inst.arg

But it never stopped in datalflow.py when executing the tests in this PR.

@esc esc Feb 23, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So, dataflow.py is for older Pythons.

@sklam sklam Feb 23, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, for python<3.7. I can't wait to drop py3.6.

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 3 - Ready for Review 4 - Waiting on author Waiting for author to respond to review labels Feb 23, 2021
@esc esc added Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Feb 23, 2021
@esc

esc commented Feb 23, 2021

Copy link
Copy Markdown
Member

Build farm ID: numba_smoketest_cpu_91

@esc esc added 5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed 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 Feb 23, 2021
@esc

esc commented Feb 23, 2021

Copy link
Copy Markdown
Member

Smoketest was green, this is good to go in.

@sklam
sklam merged commit 4ddedd6 into numba:master Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Assignees

@esc esc

Labels

5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed Effort - medium Medium size effort needed

Projects

None yet

Milestone

PR Backlog

Development

Successfully merging this pull request may close these issues.

Support for dictionary comprehensions.

3 participants

@stuartarchibald @esc @sklam