Skip to content

Fixed issue in notebook with TenosrArray bool selection #165

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

Merged
merged 6 commits into from
Jan 25, 2021

Conversation

BryanCutler
Copy link
Member

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB


# Test Series of TensorDType selection with TensorArray
# Currently fails due to Pandas not recognizing as bool index GH#162
with self.assertRaises(Exception):
Copy link
Member Author

Choose a reason for hiding this comment

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

Looking into a fix upstream, I think pandas should be able to recognize as a bool index

Copy link
Member

@frreiss frreiss 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 to me. Some minor comments inline.

@@ -35,6 +35,7 @@ class TensorDtype(pd.api.extensions.ExtensionDtype):
"""
Pandas data type for a column of tensors with the same shape.
"""
base = None
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? Should this change be repeated on the dtypes for span arrays?

Copy link
Member Author

@BryanCutler BryanCutler Jan 22, 2021

Choose a reason for hiding this comment

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

I'm not sure, I can't find any documentation for it. It's in some extension types and only set to something for a couple. Without it, there is an error in a cython function when using integer as index. I'll followup with a question to the mailing list and try to add document or a fix upstream. It doesn't seem to be needed any other place, so I wouldn't add to other arrays just yet.

I made #166 to track investigation

@BryanCutler
Copy link
Member Author

Thanks @frreiss, I'll work on an update then merge.

@BryanCutler BryanCutler merged commit bcaa55d into CODAIT:master Jan 25, 2021
@BryanCutler BryanCutler deleted the tensor-bool-selection-fix branch January 25, 2021 21:48
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.

2 participants