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

ENH: Add calamite engine to read_excel #50581

Closed
wants to merge 48 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
30da9a4
ENH: add calamite excel reader and modify test to include engine
kostyafarber Jan 5, 2023
a47d3fb
Merge branch 'main' into issue-50395
kostyafarber Jan 5, 2023
6c1dd87
Merge branch 'main' into issue-50395
kostyafarber Jan 5, 2023
fd06ad9
fix deps for python-calamine
kostyafarber Jan 5, 2023
8b6200a
Merge branch 'main' into issue-50395
kostyafarber Jan 5, 2023
6a8d822
fix deps for python-calamine, add as pip package
kostyafarber Jan 5, 2023
efcb2fc
ENH: fix typo in engine declaration, add import_optional_dependency, …
kostyafarber Jan 7, 2023
e1105de
Merge branch 'main' into issue-50395
kostyafarber Jan 11, 2023
6b50e0c
calamite -> calamine, updated some tests for calamine
dimastbk Jan 11, 2023
0784733
calamine excel engine: skip tests with datetime
dimastbk Jan 12, 2023
5971199
Merge branch 'main' into issue-50395
kostyafarber Jan 12, 2023
655318b
Merge branch 'issue-50395' into issue-50395-dima
dimastbk Jan 18, 2023
cc049cf
Merge branch 'main' into issue-50395
kostyafarber Jan 20, 2023
038133e
ENH: change reader filename match library, fix typo in engine name in…
kostyafarber Jan 22, 2023
52c2cbd
Merge branch 'issue-50395' into issue-50395-dima
kostyafarber Jan 22, 2023
2dc5e02
Merge pull request #1 from dimastbk/issue-50395-dima
kostyafarber Jan 22, 2023
2076e11
Merge pull request #2 from dimastbk/issue-50395-dima-skip-tests
kostyafarber Jan 22, 2023
6b0a7ac
Merge branch 'main' into issue-50395
kostyafarber Jan 22, 2023
a614089
ENH: add back get_sheet_by_index
kostyafarber Jan 22, 2023
256f9f9
Merge branch 'main' into issue-50395
kostyafarber Jan 23, 2023
eee8b4e
Merge branch 'main' into issue-50395
kostyafarber Jan 23, 2023
9fc2209
ENH: fix mypy and trailing whitespace
kostyafarber Jan 24, 2023
cf1268a
Merge branch 'main' into issue-50395
kostyafarber Jan 24, 2023
bebfec5
Merge branch 'main' into issue-50395
kostyafarber Jan 24, 2023
9019904
added conversion date/time/float, support file_rows_needed, fixed sup…
dimastbk Jan 26, 2023
08a5616
Update test_readers.py
dimastbk Jan 26, 2023
677a224
Merge pull request #3 from dimastbk/issue-50395
kostyafarber Jan 26, 2023
8c55e5d
Merge branch 'main' into issue-50395
kostyafarber Jan 26, 2023
d817999
update python-calamine to 0.0.7
dimastbk Jan 27, 2023
12aaf19
Merge branch 'main' into issue-50395
kostyafarber Jan 28, 2023
255e8fb
Merge pull request #4 from dimastbk/issue-50395
kostyafarber Jan 28, 2023
500fa9f
Merge branch 'main' into issue-50395
kostyafarber Jan 29, 2023
5d94728
fix review: use CalamineReader/CalamineSheet
dimastbk Jan 31, 2023
15874c3
fixed pyright, fixed docs in __init__
dimastbk Feb 6, 2023
89ae49e
Merge pull request #5 from dimastbk/issue-50395
kostyafarber Feb 7, 2023
33e5b7e
Merge branch 'main' into issue-50395
kostyafarber Feb 7, 2023
85d31ec
Merge branch 'main' into issue-50395
kostyafarber Mar 23, 2023
a0d4193
Merge branch 'main' into issue-50395
kostyafarber Mar 25, 2023
a6b6fb2
bump python-calamine to 0.1.0
dimastbk Mar 26, 2023
0a431c5
_ValueT -> _CellValueT
dimastbk Mar 29, 2023
745cd09
Merge pull request #6 from dimastbk/issue-50395
kostyafarber Mar 29, 2023
942a16a
Merge branch 'main' into issue-50395
kostyafarber Apr 1, 2023
8803ca9
Merge branch 'main' into issue-50395
kostyafarber Apr 2, 2023
2f5ffba
added xfail to tests, small fixes
dimastbk Apr 3, 2023
b8b1a9a
Merge pull request #7 from dimastbk/issue-50395
kostyafarber Apr 8, 2023
f5ab40d
Merge branch 'main' into issue-50395
kostyafarber Apr 8, 2023
02c2e7f
bump calamine to 0.1.1, update tests (472 passed, 75 xfailed), update…
dimastbk May 1, 2023
74a3e70
Merge pull request #8 from dimastbk/issue-50395
kostyafarber May 1, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix review: use CalamineReader/CalamineSheet
  • Loading branch information
dimastbk committed Feb 6, 2023
commit 5d94728efeaa9ee137573fde0bf53f41933f8ae9
2 changes: 1 addition & 1 deletion ci/deps/actions-38-minimum_versions.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,4 @@ dependencies:

- pip:
- pyqt5==5.15.1
- python-calamine==0.0.7
- python-calamine==0.0.8
2 changes: 1 addition & 1 deletion pandas/compat/_optional.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"pyarrow": "6.0.0",
"pyreadstat": "1.1.2",
"pytest": "7.0.0",
"python-calamine": "0.0.7",
"python-calamine": "0.0.8",
"pyxlsb": "1.0.8",
"s3fs": "2021.08.0",
"scipy": "1.7.1",
Expand Down
38 changes: 14 additions & 24 deletions pandas/io/excel/_calaminereader.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,7 @@
ValueT = Union[int, float, str, bool, time, date, datetime]
Copy link
Member

Choose a reason for hiding this comment

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

Can we stick with the same Scalar import that the other readers use?

Copy link
Contributor

@dimastbk dimastbk Feb 8, 2023

Choose a reason for hiding this comment

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

This type is for internal using in _convert_cell. get_sheet_data returns Scalar.

Copy link
Member

Choose a reason for hiding this comment

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

Can you move this into the function it is actually used in? Sounds like it has a pretty localized use, so no need to be in the global namespace

Copy link
Contributor

Choose a reason for hiding this comment

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

As I see in https://peps.python.org/pep-0613/#scope-restrictions, typealiases can't defined within function scope. But, I can make ValueT is protected in module level.

Copy link
Member

Choose a reason for hiding this comment

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

Ah fair point. At the very least I think a more descriptive name than ValueT would be better. What is this supposed to represent? Some type of cell scalar?

Copy link
Contributor

Choose a reason for hiding this comment

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

Renamed in _CellValueT.



class __calamine__:
pass


class CalamineExcelReader(BaseExcelReader):
book: str
_sheet_names: list[str] | None = None

def __init__(
Expand All @@ -44,10 +39,12 @@ def __init__(
super().__init__(filepath_or_buffer, storage_options=storage_options)

@property
def _workbook_class(self) -> type[__calamine__]:
return __calamine__
def _workbook_class(self):
Copy link
Member

Choose a reason for hiding this comment

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

The protocol must not be explicitly stated in code, but whatever is returned here is supposed to represent the concept of a Workbook. Not very familiar with calamine but the name CalamineReader sounds more responsible for loading data than representing a workbook concept

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

from python_calamine import CalamineReader

return CalamineReader

def load_workbook(self, filepath_or_buffer) -> str:
def load_workbook(self, filepath_or_buffer):
if hasattr(filepath_or_buffer, "read") and hasattr(filepath_or_buffer, "seek"):
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 you can get rid of all of this if _workbook_class is properly implemented; looks like the base class should handle things gracefully

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

ext = inspect_excel_format(filepath_or_buffer)
with NamedTemporaryFile(suffix=f".{ext}", delete=False) as tmp_file:
Expand All @@ -59,29 +56,24 @@ def load_workbook(self, filepath_or_buffer) -> str:

assert isinstance(filepath_or_buffer, str)

from python_calamine import get_sheet_names
from python_calamine import CalamineReader

self._sheet_names = get_sheet_names(filepath_or_buffer)
return filepath_or_buffer
return CalamineReader.from_path(filepath_or_buffer)

@property
def sheet_names(self) -> list[str]:
from python_calamine import get_sheet_names
return self.book.sheet_names

if self._sheet_names is None:
self._sheet_names = get_sheet_names(self.book)
return self._sheet_names

def get_sheet_by_name(self, name: str) -> int:
def get_sheet_by_name(self, name: str):
self.raise_if_bad_sheet_by_name(name)
return self.sheet_names.index(name)
return self.book.get_sheet_by_name(name)

def get_sheet_by_index(self, index: int) -> int:
def get_sheet_by_index(self, index: int):
self.raise_if_bad_sheet_by_index(index)
return index
return self.book.get_sheet_by_index(index)

def get_sheet_data(
self, sheet: int, file_rows_needed: int | None = None
self, sheet, file_rows_needed: int | None = None
) -> list[list[Scalar]]:
def _convert_cell(value: ValueT) -> Scalar:
if isinstance(value, float):
Expand All @@ -97,9 +89,7 @@ def _convert_cell(value: ValueT) -> Scalar:

return value

from python_calamine import get_sheet_data

rows = get_sheet_data(self.book, sheet, skip_empty_area=False)
rows: list[list[ValueT]] = sheet.to_python(skip_empty_area=False)
data: list[list[Scalar]] = []

for row in rows:
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ all = ['beautifulsoup4>=4.9.3',
'pytest>=7.0.0',
'pytest-xdist>=2.2.0',
'pytest-asyncio>=0.17.0',
'python-calamine>=0.0.7',
'python-calamine>=0.0.8',
'python-snappy>=0.6.0',
'pyxlsb>=1.0.8',
'qtpy>=2.2.0',
Expand Down