Skip to content
This repository has been archived by the owner on Dec 11, 2023. It is now read-only.

index_as_columns option in ctable.fromdataframe() #345

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

Conversation

rytsim
Copy link

@rytsim rytsim commented May 2, 2017

Improves on #344

Makes df -->ct-->df rountrip possible.

  • Added an option index_as_columns to save the index of a dataframe in ctable.fromdataframe() method.
  • Changed the ctable.todataframe() method to check for dataframe_index_columns attribute and if found set the dataframe index to this column before returning it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 65.303% when pulling cf62f31 on rytsim:master into 8df64b4 on Blosc:master.

@@ -644,13 +644,17 @@ def copy(self, **kwargs):
return ccopy

@staticmethod
def fromdataframe(df, **kwargs):
def fromdataframe(df, index_as_columns=False, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Although backward incompatible, I'd say that setting index_as_columns to True as default makes more sense.

Copy link
Author

Choose a reason for hiding this comment

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

I had the backward compatibility in mind, but I do agree that index_as_columns to True as a default makes more sense.

# In Python 3 strings should be represented as Unicode
dtype = "U" if sys.version_info >= (3, 0) else "S"
col = bcolz.carray(vals, dtype='%s%d' %
(dtype, maxitemsize), **ckwargs)
Copy link
Member

Choose a reason for hiding this comment

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

I think that 'unicode' and 'string' branches can be fused a bit more.

@FrancescAlted
Copy link
Member

Looks good to me; I have added a couple of comments though. Also, a couple of test units would be needed.

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants