Add support for lists in kwargs (in operations) #23
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
==========================================
+ Coverage 96.74% 96.84% +0.10%
==========================================
Files 12 12
Lines 1596 1615 +19
==========================================
+ Hits 1544 1564 +20
+ Misses 52 51 -1
Continue to review full report at Codecov.
|
| @@ -1,10 +1,11 @@ | |||
| # Generated from blackbird.g4 by ANTLR 4.7.1 | |||
| # Generated from src/blackbird.g4 by ANTLR 4.8 | |||
There was a problem hiding this comment.
Out of curiosity, does this affect the version of antlr4-python3-runtime we are currently pinning in setup.py and requirements.txt? We're currently pinning 4.7.1, but this might require us to upgrade the requirements
There was a problem hiding this comment.
I updated this in the requirements since I happened to use antlr4-python3-runtime=4.8 while generating the files. Could there be any other issues with this that I'm missing?
There was a problem hiding this comment.
Nope, should be fine! I shouldn't have commented until I read the PR to the end 😆
There was a problem hiding this comment.
Nice work @thisac! ANTLR is not the easiest library to get installed, let alone work with.
Your solution looks clear and concise as well 👍 This doesn't allow lists for args, but I don't think that's too much of an issue (I don't think there is any case where an non-named argument will be passed as a list).
Couple of requests, however:
-
Don't forget to update
setup.pywith the newantlr4-python3-runtimeversion! -
Might be worth also testing the case
dark_counts=[1], to ensure that it works correctly for a list of length 1 -
In addition, it would be good to test an operation that contains a combination of args, kwargs, and kwargs with lists. If none exists, I think it's fine just to make one up for the test.
| def test_operation_kwarglist(self, parse_input_mocked_metadata): | ||
| """Test that an operation with keyword arguments is correctly parsed""" | ||
| bb = parse_input_mocked_metadata("MeasureFock(dark_counts=[1, 3]) | [0, 1]\n") | ||
| assert bb.operations == [ | ||
| {"modes": [0, 1], "op": "MeasureFock", "args": [], "kwargs": {"dark_counts": [1, 3]}} | ||
| ] |
| @@ -1,4 +1,4 @@ | |||
| antlr4-python3-runtime==4.7.2 | |||
| antlr4-python3-runtime==4.8 | |||
There was a problem hiding this comment.
Answered my question from above 🙂
Thanks @josh146. I deliberately chose not to implement list support for args, since I'm not sure if that's currently needed or wanted. I chose to focus on the specific problem with the
Done! I expanded the tests to include testing single element lists and empty lists, as well as combinations of args, kwargs and kwargs with lists. |
|
@thisac: I had a look at the coverage report. My initial instinct was that Codecov was messing up, but it looks like these two lines are not covered: elif v.nonnumeric():
kwargslist.append(_literal(v.nonnumeric()))It should be sufficient to simply add lists that contain bools or strings to your parametrized |
|
Thanks @josh146 for finding the testing issue. I looked at it quickly before and also thought it was Codecov doing weird things again. Added a small variety of cases with strings and bools in the lists. Seems to pass now! 😄 |
Context: Currently Blackbird doesn't support lists in keyword arguments in operations which causes problems with e.g. the
selectanddark_counts; for example,MeasureHomodyne(select=[0, 1])orMeasureFock(dark_counts=[1, 0.5, 3]).Description: A new parser rule called
vallistis added, and thekwargparser rule is updated to support it._get_argumentsinauxiliary.pyis also updated to support the handling of lists as keyword arguments.Benefits: Lists can now be used in operations as values for keyword arguments. This allows e.g. the loading and parsing of Blackbird scripts containing this.
Drawbacks: N/A