Skip to content

BUG: declare and use correctly self.unique_check #10139

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

Closed
wants to merge 1 commit into from

Conversation

toobaz
Copy link
Member

@toobaz toobaz commented May 14, 2015

I am not sure I understood the last comment in the previous pull request, but anyway this is a rebased version.

In the previous PR I proposed to simply make the __get__ of is_unique directly rely on self.initialized, but I am no more so sure it is a good idea, since the current status is slightly redundant but clearer (and coherent with the use of "is_monotonic").

@jreback
Copy link
Contributor

jreback commented May 16, 2015

ok, what exactly does this fix?

@toobaz
Copy link
Member Author

toobaz commented May 17, 2015

First, self.unique_check (which follows the same logic of self.monotonic_check) is now initialized.

Then, when is_unique is accessed, self.unique_check is checked. But,self.unique_check is (was) not set when the index is not unique. So on a non-unique index, every access to is_unique results in a call to _do_unique_check, which calls then _ensure_mapping_populated, which finally does nothing (in calls following the first) because self.initialized is now 1. So the performance loss is minimal, but still it doesn't make any sense.

@jreback
Copy link
Contributor

jreback commented May 18, 2015

is unique_check really a duplicate of initialized? if so, just remove it. check that everything passes and perf is unchanged.

@toobaz
Copy link
Member Author

toobaz commented May 29, 2015

See #10229

@toobaz toobaz closed this May 29, 2015
@jorisvandenbossche
Copy link
Member

@toobaz just for future reference, you don't have to open a new PR every time you update. Just update your branch and push again will do!

@toobaz
Copy link
Member Author

toobaz commented May 29, 2015

@jorisvandenbossche : since I was indeed trying to understand this... by "update" do you mean "push a new commit" or "rebase and push -f"? Because I tried the latter and it didn't seem to work (but I may have been mistaken).

@jorisvandenbossche
Copy link
Member

As long as you stay on the same branch, it does not matter. Adding a new commit, rebasing, etc all work and the PR will be updated with that if you push (force) again.

@jorisvandenbossche
Copy link
Member

But I see that the new PR uses another branch name. So you only need to stay on the same branch to update the PR

@toobaz
Copy link
Member Author

toobaz commented May 29, 2015

I changed branch name precisely because I didn't think using the same would work! Thanks for the tip.

@jreback jreback added the Clean label Jun 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants