-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
API: allow Series comparison ops to align before comparison (GH1134) #6860
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
Conversation
IIRC we discussed this ad nauseum before. Its more 'correct' for the missing values to return Furthermore, a reordered Series is really NOT equal. |
This is from test_json/test_ujson/testSeries This is a failing test with this PR, because before the values DO compare correctly if you didn't align the indexes. Aligning causes this to fail (as nothing matches up as 1 is Int64, the other object). This DOES look correct though as deserializing does not guarantee that something that looks like a numerical index is actually numerical, right? (except for DatetimeIndex and we have a separate kw arg fo that). right?
|
Yeah the problem with JSON is keys must be strings so when you read them back you really have no idea without doing some guesswork (which is what The lower level 'decode' method which this is testing gives you a string index back, which didn't matter during comparison before as there was no alignment happening. Your fix looks good, although it would work just as well I think to change the test Series to have a string index from the start e.g. In [20]: s = Series([10, 20, 30, 40, 50, 60], name="series", index=[str(s) for s in [6,7,8,9,10,15]])
In [26]: Series(ujson.decode(ujson.encode(s))).index
Out[26]: Index([u'10', u'15', u'6', u'7', u'8', u'9'], dtype='object') |
@jreback Just wondering, but it would also be an option to only let Because with this change you have |
+1 on @jorisvandenbossche's suggestion: named methods flexible, corresponding syntax is not. I personally find the unaligned error a useful sanity check. |
going to bump; can work on in next version |
closes #1134
reordered comparisons
Here we have a missing value, so it's
nan
in the comparisons