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

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Feb 13, 2017

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

@philippjfr philippjfr commented Feb 13, 2017

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

@jlstevens jlstevens commented Feb 13, 2017

Happy to merge once the tests pass.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Feb 13, 2017

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

@jlstevens jlstevens commented Feb 13, 2017

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 philippjfr force-pushed the annotation_length branch from 3fe9d39 to ead8102 Feb 13, 2017
@philippjfr philippjfr force-pushed the annotation_length branch from ead8102 to c47e842 Apr 12, 2017
@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Apr 14, 2017

@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 philippjfr force-pushed the annotation_length branch from fcf1dc9 to 29d528b Apr 14, 2017
@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Apr 14, 2017

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 Apr 14, 2017
@philippjfr philippjfr force-pushed the annotation_length branch from 67397ae to ae3c972 Apr 14, 2017
@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Apr 14, 2017

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

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Apr 14, 2017

Yes, I think it's ready now.

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Apr 14, 2017

Tests have all passed now. Merging.

@jlstevens jlstevens merged commit 16a6aea into master Apr 14, 2017
4 checks passed
4 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.1%) to 78.767%
Details
@philippjfr
s3-reference-data-cache Test data is cached.
Details
@philippjfr philippjfr deleted the annotation_length branch Apr 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants