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

Define length on Annotation Elements #1133

Merged
merged 7 commits into from
Apr 14, 2017
Merged

Define length on Annotation Elements #1133

merged 7 commits into from
Apr 14, 2017

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Feb 13, 2017

Defining a length is part of the core API and I have run into failures which I cannot reproduce right now because the annotations do not support it. Setting length 1 is consistent with how their dimension_values methods work, which return length 1 arrays.

Edit: I threw a bug fix for a small bug on the Arrow Annotation

@philippjfr philippjfr added tag: API type: bug Something isn't correct or isn't working labels Feb 13, 2017
@jlstevens
Copy link
Contributor

A length of 1 seems reasonable. If __len__ is now required, why not raise a NotImplementedError in the appropriate base class (maybe Element?)

@philippjfr
Copy link
Member Author

I just added a general __len__ implementation for Element which is based on the length of dimension_values along the first dimension. This should work fine because dimension_values HAS to be implemented and the length of the element is always defined as the length of its dimension_values. Subclasses can override for more efficient implementations.

@jlstevens
Copy link
Contributor

Happy to merge once the tests pass.

@philippjfr
Copy link
Member Author

Hmm, so this is problematic, turns out something is actively looking for the __len__ attribute and enabling checks in that case. Since Element doesn't actually have any values but is used all over the tests this now fails. Making it NotImplemented would also fail, really not sure what the best solution is.

@jlstevens
Copy link
Contributor

Hmm, so this is problematic, turns out something is actively looking for the len attribute

My initial reaction is this could be due to testing of the boolean value of elements or pretty printing...

@philippjfr
Copy link
Member Author

@jlstevens I think this is the correct fix now, I'll still try to reproduce the original bug so I can add a unit test.

@philippjfr
Copy link
Member Author

I also just found out that cloning of Text and Arrow types was broken, so any deep selection, redim, relabel etc. was breaking. I've now addressed this by reordering the .data of these types to match the signature and implementing .clone to handle that.

@philippjfr philippjfr force-pushed the annotation_length branch 2 times, most recently from 45be77a to 67397ae Compare April 14, 2017 13:58
@jlstevens
Copy link
Contributor

Other than one transient error, tests are passing. Ready to merge?

@philippjfr
Copy link
Member Author

Yes, I think it's ready now.

@jlstevens
Copy link
Contributor

Tests have all passed now. Merging.

@jlstevens jlstevens merged commit 16a6aea into master Apr 14, 2017
@philippjfr philippjfr deleted the annotation_length branch April 19, 2017 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: API type: bug Something isn't correct or isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants