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

RFC: Re-enable axis checks in constructor #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Jun 11, 2017

This is a major breaking change. Back before this package was officially registered, these checks were dropped. That means that many of the indexing methods that require sorted vectors are now broken. Enforcing these preconditions in the constructor is, unfortunately, terribly breaking.

I think the (slightly friendlier) alternative here would be to delay the checks until indexing occurs, but the downside there is that for non-range axes, it would require a issorted pass through the data upon every operation. We could defer to the SortedVector type more, but that easily walks us into a type-unstable API. Thoughts?

This is a major breaking change. Back before this package was officially registered, these checks were dropped. That means that many of the indexing methods that require sorted vectors are now broken. Enforcing these preconditions in the constructor is, unfortunately, terribly breaking.

I think the alternative here would be to delay the checks until indexing occurs, but the downside there is that for non-range axes, it would require a `issorted` pass through the data upon every operation. We could defer to the SortedVector type more, but that easily walks us into a type-unstable API. Thoughts?
@timholy
Copy link
Member

timholy commented Jun 11, 2017

Personally I like the idea of running the checks upon construction. Is there really a use case for non-monotonic axes?

@iamed2
Copy link
Collaborator

iamed2 commented Jun 15, 2017

@timholy yes there is: #88

I agree that it's not an optimal code path but when you care about the order of your axis elements and that order is different than a sorting order, it's nice to be able to still use AxisArrays. This only applies in the Categorical case though.

@timholy
Copy link
Member

timholy commented Jun 15, 2017

Thanks for explaining. Sure, I agree that for categorical axes we might want to allow non-monotonicity, since there isn't really a useful notion of "range" anyway.

@timholy
Copy link
Member

timholy commented Jun 23, 2017

I rebased #88 on top of this and verified that this doesn't break anything.

My plan is to merge this, but AFTER we figure out #90 since I'm guessing folks would not like to leave a broken package on 0.5, and because making a breaking change like this is a good opportunity to require 0.6 as a minimum.

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.

3 participants