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

BUG: raise on frozenset construction by Series #4482

Merged
merged 1 commit into from Aug 8, 2013
Merged

BUG: raise on frozenset construction by Series #4482

merged 1 commit into from Aug 8, 2013

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Aug 6, 2013

No description provided.

@ghost
Copy link

ghost commented Aug 6, 2013

  1. Argument from simplicity - Pandas data structures are ordered and the user is expected to provide an ordered (or keyed) data structure as input. It's simple and logical, I don't think there's a need to change it.
  2. Argument from necessity - What's the problem this actually fixes? that the user has to pass in list(values) rather then values? meh.
  3. Argument from consequence - Slippery slope, once you introduce set to the series ctor you break consistency (and an established pandas rule of thumb) with dataframe and panel. a small break in
    consistency can spread far and wide across a codebase when later attempts try to regain it. This may be FUD, but I just don't think it's worth it.

I'm -1 on this.

@cpcloud
Copy link
Member Author

cpcloud commented Aug 6, 2013

Your argument about consistency is the most convincing to me, but the original thought was: if list can take a set why can't Series? That would make Series more consistent with Python, but of course breaks it with pandas. I agree with you that we should keep consistency with the latter.

I agree with 1. and 2.

I don't feel strongly about this .... I've never actually run into the issue before....

At the very least this should raise on frozenset objects with the same error.

@cpcloud
Copy link
Member Author

cpcloud commented Aug 6, 2013

cc @hayd since he might have a stronger opinion than I about this.

@hayd
Copy link
Contributor

hayd commented Aug 6, 2013

Not really strong... the list argument is all I really have.

I think passing in any iterator should be ok... :s

In [11]: s = set([2])

In [12]: pd.Series(iter(s))
Out[12]: <setiterator at 0x104559280>

In [13]: pd.Series(iter(list(s)))
Out[13]: <listiterator at 0x104554b90>

In [14]: pd.Series(i for i in s)
Out[14]:
0    2
dtype: int64

so don't see why iterables should be so different, and why we should stop it (just cos we can't test it) but... meh.

Interested to see if/when dictionaries have different ordering for different python versions!

@ghost
Copy link

ghost commented Aug 6, 2013

Good example. That's actually my 3rd point in action: a while ago generators were accepted
into the ctor (to save users the anguish of list(gen)) and now you're showing the inconsistency
it generated as an (reasonable) argument to support even more stuff.

I think it's fine to support iterators as you suggest (spilled generator milk, so to speak), but that means iter(set) not set,
so that's really a separate issue your example raises. iterat-or and an iter-able are
different beasts I believe, am I wrong?

re different ordering for different python versions, how about different ordering in the same
python version courtesy of hash randomization?

λ python3.3 -c 'print(dict(a=1,b=2))'
{'b': 2, 'a': 1}
λ python3.3 -c 'print(dict(a=1,b=2))'
{'a': 1, 'b': 2}

@cpcloud
Copy link
Member Author

cpcloud commented Aug 6, 2013

@y-p nice example.

yep iterables and iterators are different, iter(list('abc')) != list('abc')

@hayd also generators are yet another type of iterable which is why you get the output above

@hayd
Copy link
Contributor

hayd commented Aug 7, 2013

woah! that is a nice example :) I'm sold.

Maybe error could mention "first use list or sorted" ?

result = store.select('df',Term('x!=none'))
expected = df[df.x != 'none']
assert_frame_equal(result,expected)
except Exception as detail:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpcloud I guess will just leave this in here...and see what happens

@cpcloud
Copy link
Member Author

cpcloud commented Aug 7, 2013

@jreback good 2 go after passing?

@jreback
Copy link
Contributor

jreback commented Aug 7, 2013

thought u closed this? obviously not...ok

cpcloud added a commit that referenced this pull request Aug 8, 2013
BUG: raise on frozenset construction by Series
@cpcloud cpcloud merged commit 5a2daa4 into pandas-dev:master Aug 8, 2013
@cpcloud cpcloud deleted the set-construct branch August 8, 2013 00:14
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.

None yet

4 participants