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

DataFrame.to_excel #288

Merged
merged 19 commits into from
May 11, 2019
Merged

DataFrame.to_excel #288

merged 19 commits into from
May 11, 2019

Conversation

shril
Copy link
Contributor

@shril shril commented May 9, 2019

No description provided.

@garawalid
Copy link
Contributor

@shril you need to include openpyxl in requirements-dev.txt

# Dependencies in Koalas
pandas>=0.23
pyarrow>=0.10,<0.11
numpy>=0.14
openpyxl

@shril
Copy link
Contributor Author

shril commented May 9, 2019

@shril you need to include openpyxl in requirements-dev.txt

# Dependencies in Koalas
pandas>=0.23
pyarrow>=0.10,<0.11
numpy>=0.14
openpyxl

Just figured that out. Btw thanks @garawalid. :)

kdf = self.kdf
excel_writer = "output.xlsx"

self.assert_eq(kdf.to_excel(excel_writer), pdf.to_excel(excel_writer))
Copy link
Contributor

Choose a reason for hiding this comment

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

should we assert the output.xlsx is the same, rather than asserting on the return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rxin, should I load the values of the generated excel files in 2 dataframes and check the values?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that'd work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you write it to a temporary location that's unique too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rxin, I have made the changes you asked for.
Please have a look.

@garawalid
Copy link
Contributor

@shril you are welcome!

@codecov-io
Copy link

codecov-io commented May 9, 2019

Codecov Report

Merging #288 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
+ Coverage   93.49%   93.56%   +0.07%     
==========================================
  Files          33       33              
  Lines        3212     3247      +35     
==========================================
+ Hits         3003     3038      +35     
  Misses        209      209
Impacted Files Coverage Δ
databricks/koalas/missing/frame.py 100% <ø> (ø) ⬆️
...tabricks/koalas/tests/test_dataframe_conversion.py 100% <100%> (ø) ⬆️
databricks/koalas/frame.py 95.07% <100%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ad42d3...70d6b0c. Read the comment docs.

if os.path.isfile(location2):
os.remove(location2)
if os.path.exists(directory):
os.rmdir(directory)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use TestUtils.temp_dir or TestUtils.temp_file.

class DataFrameConversionTest(ReusedSQLTestCase, SQLTestUtils, TestUtils):

    ...

    def test_xxx(self):
        with self.temp_dir() as tmp:
            ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ueshin, I have changed it according to your suggestions.
Please have a look.

requirements-dev.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@ueshin ueshin left a comment

Choose a reason for hiding this comment

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

LGTM pending tests.

@shril
Copy link
Contributor Author

shril commented May 11, 2019

Hi @ueshin, I have changed according to your specifications.
Please have a look.

@ueshin
Copy link
Collaborator

ueshin commented May 11, 2019

Thanks! merging to master.

@ueshin ueshin merged commit 3f66f5e into databricks:master May 11, 2019
athena15 pushed a commit to athena15/koalas that referenced this pull request May 13, 2019
* DataFrame.to_excel

* Lint fix

* Lint fix

* Lint fix

* Added install of openpxyl as excel engine

* Comparing the values of generated excel files

* Added xlrd as dependecncy

* Added xlrd as dependecncy

* Refactored the code

* Moving the libraries to test suite

* Used TestUtils functions for creating and removing files and folder

* Lint fix

* Lint fix

* Added precautionary note in inline doc of DataFrame.to_excel

* Using temp_dir function of TestUtils for creating temporary folder

* Removing redundant library

* Removing redundant library
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.

6 participants