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

Rectify Return type hint #6811

Closed
Tracked by #7053
Dhruvanshu-Joshi opened this issue Jul 3, 2023 · 4 comments · Fixed by #7125
Closed
Tracked by #7053

Rectify Return type hint #6811

Dhruvanshu-Joshi opened this issue Jul 3, 2023 · 4 comments · Fixed by #7125

Comments

@Dhruvanshu-Joshi
Copy link
Member

Dhruvanshu-Joshi commented Jul 3, 2023

Description

Take a look at the code snippet below:

def find_measurable_comparisons(
fgraph: FunctionGraph, node: Node
) -> Optional[List[MeasurableComparison]]:

Here, the return type hint is wrong. MeasurableComparision is an Op, but what is returned are TensorVariables whose node has that Op.

This needs to be rectified for all the functions which intend to find a measurable op.

CC @ricardoV94 @larryshamalama

@5hv5hvnk
Copy link
Member

5hv5hvnk commented Jul 7, 2023

@larryshamalama
Just to be sure a fix would be returning the node or changing the return data type?

@larryshamalama
Copy link
Member

larryshamalama commented Oct 21, 2023

@larryshamalama Just to be sure a fix would be returning the node or changing the return data type?

Yes, I believe just changing the return data type would be good. Sorry, it looks like I missed your question from a while ago...

@vivek-anand-singh
Copy link
Contributor

It seems like an easy fix. We just need to change variable "MeasurableComparison" in return type to variable "TensorVariable".

Here is the code modifications:

def find_measurable_comparisons(
    fgraph: FunctionGraph, node: Node
) -> Optional[List[TensorVariable]]:

Could you please verify if this is corrent? I can open a pull request for it.

@larryshamalama
Copy link
Member

It seems like an easy fix. We just need to change variable "MeasurableComparison" in return type to variable "TensorVariable".

Here is the code modifications:

def find_measurable_comparisons(
    fgraph: FunctionGraph, node: Node
) -> Optional[List[TensorVariable]]:

Could you please verify if this is corrent? I can open a pull request for it.

That should be the case! And that would be great :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants