Add type hints in `pulser.parametrized` [unitaryHACK] (#16) by TripleRD · Pull Request #175 · pasqal-io/Pulser

TripleRD · 2021-05-21T13:11:34Z

Type checked on decorators.py, paramabc.py, paramobj.py and variable.py

I have some doubts about the return type of the build function.

HGSilveri · 2021-05-21T13:20:09Z

I have some doubts about the return type of the build function.

The build method is complicated, it's probably better to leave it without type-hints in Parametrized and ParamObj. We can probably type hint it in Variable, though.

I'll review this asap. Make sure to address all the errors in the CI tests in the mean time.

TripleRD · 2021-05-21T13:40:18Z

I have some doubts about the return type of the build function.

The build method is complicated, it's probably better to leave it without type-hints in Parametrized and ParamObj. We can probably type hint it in Variable, though.

I'll review this asap. Make sure to address all the errors in the CI tests in the mean time.

Okay, have corrected the errors in the CI test, updating the type hints on build.

HGSilveri

Alright, here are a few comments. Try to be more specific when you can (e.g. if you know the type of the keys and values of a dict, type-hint them too).

…pe-hint-parametrized

…/Pulser into type-hint-parametrized

TripleRD · 2021-05-21T16:41:29Z

I'll review this asap. Make sure to address all the errors in the CI tests in the mean time.

Okay @HGSilveri, I'm on it.

Updates -

Untyped the build function from Parametrized and ParamObj
Changed few type hints
Wasn't able to solve pulser/tests/test_parametrized.py:31: error: List or tuple expected as variable arguments. Went through the function definition but didn't find any issue.

Pulser/pulser/tests/test_parametrized.py

Line 31 in e89ef2f

pulse = Pulse.ConstantDetuning(bwf, *b)

HGSilveri · 2021-05-21T17:28:29Z

Wasn't able to solve pulser/tests/test_parametrized.py:31: error: List or tuple expected as variable arguments. Went through the function definition but didn't find any issue.

Pulser/pulser/tests/test_parametrized.py

Line 31 in e89ef2f

pulse = Pulse.ConstantDetuning(bwf, *b)

Try to change that line to

pulse = Pulse.ConstantDetuning(bwf, b[0], b[1])

TripleRD · 2021-05-21T20:16:15Z

Hey @HGSilveri, I have updated the suggested changes.

HGSilveri

Alright, it's getting there!

TripleRD · 2021-05-21T21:07:36Z

Done

HGSilveri

Alright, it's getting there! The next commit should do it

…/Pulser into type-hint-parametrized

TripleRD · 2021-05-22T09:08:28Z

Done

HGSilveri · 2021-05-22T11:06:34Z

 from abc import ABC, abstractmethod
+from typing import Dict, Any
+
+from pulser.parametrized import Variable


This is what giving you the test error. Try this instead:

Suggested change

from pulser.parametrized import Variable

from pulser.parametrized.variable import Variable

If this doesn't work either, there's another option, but try this first.

Ok, it didn't work. Try this then:

from typing import Dict, Any, TYPE_CHECKING if TYPE_CHECKING: from pulser.parametrized import Variable

Okay, updating it.

You have another import like this, you should change it too

HGSilveri · 2021-05-22T13:48:37Z

-from pulser.parametrized import Parametrized, Variable
 from pulser.json.utils import obj_to_dict
+if TYPE_CHECKING:
+    from pulser.parametrized import Parametrized, Variable


You can't really do this because Parametrized is needed even when you're not type checking. You have to split the import into two lines

HGSilveri · 2021-05-22T14:01:12Z

Okay, now you have a coverage problem, which is to be expected... just add #pragma: no cover on the import Variable lines

HGSilveri

LGTM, good job!

TripleRD · 2021-05-22T14:26:26Z

LGTM, good job!

Thanks for the guidance! 😃

TripleRD and others added 2 commits May 21, 2021 18:37

Add type hints in pulser.parametrized

13706a3

Merge branch 'develop' into type-hint-parametrized

72600bd

Update style and type hints in pulser.parametrized

7df5eca

HGSilveri requested changes May 21, 2021

View reviewed changes

TripleRD added 2 commits May 21, 2021 21:47

Merge branch 'develop' of https://github.com/TripleR47/Pulser into ty…

96d107f

…pe-hint-parametrized

Merge branch 'type-hint-parametrized' of https://github.com/TripleR47…

f9a9379

…/Pulser into type-hint-parametrized

TripleRD added 2 commits May 22, 2021 01:26

Update type hints in pulser.parametrized

19abeeb

Update test_parametrized.py

a16b2b8

HGSilveri requested changes May 21, 2021

View reviewed changes

Comment thread pulser/parametrized/paramobj.py Outdated

Comment thread pulser/parametrized/variable.py Outdated

Comment thread pulser/parametrized/variable.py Outdated

Update type hints in paramobj.py and variable.py

743140e

Merge branch 'develop' into type-hint-parametrized

274da15

HGSilveri requested changes May 22, 2021

View reviewed changes

Comment thread pulser/parametrized/paramabc.py

Comment thread pulser/parametrized/paramobj.py Outdated

Comment thread pulser/parametrized/paramobj.py Outdated

TripleRD added 3 commits May 22, 2021 14:14

Update type hint in paramabc.py and paramobj.py

2d591bb

Merge branch 'type-hint-parametrized' of https://github.com/TripleR47…

c8b1575

…/Pulser into type-hint-parametrized

Update in paramabc.py

67ae04c

HGSilveri reviewed May 22, 2021

View reviewed changes

TripleRD added 3 commits May 22, 2021 17:03

Update in paramabc.py

3f40dc9

Update paramabc.py

01b5bdf

Update paramobj.py

89761a8

HGSilveri reviewed May 22, 2021

View reviewed changes

Update paramobj.py

cc350a7

Update paramabc.py and paramobj.py

423639d

HGSilveri approved these changes May 22, 2021

View reviewed changes

HGSilveri merged commit ffcdf56 into pasqal-io:develop May 22, 2021

	from pulser.parametrized import Variable
	from pulser.parametrized.variable import Variable

Conversation

TripleRD commented May 21, 2021

Uh oh!

HGSilveri commented May 21, 2021

Uh oh!

TripleRD commented May 21, 2021

Uh oh!

HGSilveri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TripleRD commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HGSilveri commented May 21, 2021

Uh oh!

TripleRD commented May 21, 2021

Uh oh!

HGSilveri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TripleRD commented May 21, 2021

Uh oh!

HGSilveri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TripleRD commented May 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HGSilveri commented May 22, 2021

Uh oh!

HGSilveri left a comment

Choose a reason for hiding this comment

Uh oh!

TripleRD commented May 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TripleRD commented May 21, 2021 •

edited

Loading