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

Pcvl 749 Remove python 3.8 #459

Merged
merged 5 commits into from
Sep 23, 2024
Merged

Conversation

Benoit-F-Q
Copy link
Contributor

No description provided.

@MarionQuandela MarionQuandela changed the base branch from main to release/0.12.0 September 5, 2024 14:00
Copy link
Contributor

@raksharuia raksharuia left a comment

Choose a reason for hiding this comment

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

With @MarionQuandela, we found that some stuffs will raise errors. She will add them as comments in her review soon.

@@ -310,7 +309,7 @@
"metadata": {},
"outputs": [],
"source": [
"def mzi(name:str, theta:Union[float, pcvl.Parameter], phi:Union[float, pcvl.Parameter],theta_2:Union[float, pcvl.Parameter]) -> pcvl.Circuit:\n",
"def mzi(name:str, theta:float | pcvl.Parameter, phi:float | pcvl.Parameter, theta_2:float | pcvl.Parameter) -> pcvl.Circuit:\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

| is only working from python 3.10 and we still support python 3.9.
https://docs.python.org/3.10/whatsnew/3.10.html#pep-604-new-type-union-operator
But do not loose hope, you can add at the top of the import from __future__ import annotations (it's very important to put it as the first import) and we will delete this import in 3.10 (it's already elsewhere in the code)

Copy link
Contributor

Choose a reason for hiding this comment

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

So, is this still to be fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Marion did it

Copy link
Contributor

Choose a reason for hiding this comment

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

We're inside the pull request regarding this issue, you tell me Marion did it, but where is the diff showing the fix?
I expect from __future__ import annotations to be added somewhere on top of this file. I might be blind but I can't find it.

@@ -581,10 +580,10 @@ def compute_unitary(self,
@staticmethod
@deprecated(version="0.10.0", reason="Construct a GenericInterferometer object instead")
def generic_interferometer(m: int,
fun_gen: Callable[[int], ACircuit],
shape: Union[str, InterferometerShape] = InterferometerShape.RECTANGLE,
fun_gen: callable[[int], ACircuit],
Copy link
Contributor

@MarionQuandela MarionQuandela Sep 18, 2024

Choose a reason for hiding this comment

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

callable[something] is not a correct type for hinting, you must use from collections.abc import Callable.
This is not causing any error because it's not used anywhere.
You can see code example in perceval/components/generic_interferometer.py GenericInterferometer.__init__

@MarionQuandela
Copy link
Contributor

I this you can also remove the method _removesuffix in perceval/utils/persistent_data.py and replace it with removesuffix

@ericbrts ericbrts merged commit 221264f into Quandela:release/0.12.0 Sep 23, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants