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

Convert integer to string in operator variable represenation #1013

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

guyer
Copy link
Member

@guyer guyer commented Feb 16, 2024

Fixes #961
Addresses #1011

@tkphd
Copy link
Contributor

tkphd commented Feb 16, 2024

I'm not pythonic enough to understand what this syntax change means. It looks like you're changing the code from taking the second element of the array, to I have no idea what the [...]-th element means.

@guyer
Copy link
Member Author

guyer commented Feb 16, 2024

I'm not pythonic enough to understand what this syntax change means. It looks like you're changing the code from taking the second element of the array, to I have no idea what the [...]-th element means.

The test is asking for the represenation of the lazy evaluation of the second element in the array. Under Python 2.7, it returned Variable(value=array([1, 2, 3, 4]))[index]. (After this fix) under Python 3, it returns Variable(value=array([1, 2, 3, 4]))[1]. # doctest: +ELLIPSIS enables me to write a test that accepts either output.

The fact that humans won't know what that means is OK; this test is under _testBinOp() because it's not intended as documentation.

@tkphd
Copy link
Contributor

tkphd commented Feb 16, 2024

I'm not pythonic enough to understand what this syntax change means. It looks like you're changing the code from taking the second element of the array, to I have no idea what the [...]-th element means.

The test is asking for the represenation of the lazy evaluation of the second element in the array. Under Python 2.7, it returned Variable(value=array([1, 2, 3, 4]))[index]. (After this fix) under Python 3, it returns Variable(value=array([1, 2, 3, 4]))[1]. # doctest: +ELLIPSIS enables me to write a test that accepts either output.

The fact that humans won't know what that means is OK; this test is under _testBinOp() because it's not intended as documentation.

Ah-ha! Cool, thank you.

Copy link
Contributor

@tkphd tkphd left a comment

Choose a reason for hiding this comment

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

Looks good, per the explanation.

@guyer
Copy link
Member Author

guyer commented Feb 16, 2024

nix is the devil

@guyer guyer merged commit 3ae5bee into master Feb 16, 2024
22 of 24 checks passed
@guyer guyer deleted the issue961-Representation_of_index_variables_is_broken branch February 16, 2024 20:17
@tkphd
Copy link
Contributor

tkphd commented Feb 16, 2024

nix is the devil

and not spoken of in polite company

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

Successfully merging this pull request may close these issues.

Representation of index variables is broken
2 participants