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

Conversation

Projects
None yet
2 participants
@philippjfr
Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

jlstevens commented Feb 13, 2017

Happy to merge once the tests pass.

@philippjfr

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 14, 2017

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

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 14, 2017

Yes, I think it's ready now.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 14, 2017

Tests have all passed now. Merging.

@jlstevens jlstevens merged commit 16a6aea into master Apr 14, 2017

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
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