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

index_as_columns option in ctable.fromdataframe() #345

Open
rytsim wants to merge 1 commit intoBlosc:masterfrom
rytsim:master
Open

index_as_columns option in ctable.fromdataframe() #345
rytsim wants to merge 1 commit intoBlosc:masterfrom
rytsim:master

Conversation

@rytsim
Copy link
Copy Markdown

@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
Copy Markdown

Coverage Status

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


@staticmethod
def fromdataframe(df, **kwargs):
def fromdataframe(df, index_as_columns=False, **kwargs):
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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