ENH: Open Document Format ODS support in read_excel()
State: closed

This work should eventually lead to a reader and writer for ods spreadsheets (LibreOffice / OpenOffice Open Document Format). I am doing this work in the master of my clone of pandas. From the wiki I understood it is fine for me to commit to my master and rebase with upstream from time to time. Please let me know if I should follow another git workflow (this is my first PR).

This is what I have so far:
- It seems to make sense to implement an OdsFile and OdsWriter in pandas/io/excel.py
- I created ods test files identical to the other Excel test files.
- Basic reading works, but does not handle all corner cases correctly yet
- Tests and WritingOds are not implemented yet

I am not sure how to approach the following issues:
- For the tests, it would make sense to also use pandas/io/tests/test_excel.py (there would be a lot of code duplication otherwise). However, this would require some kind of a more elaborate test skipping mechanism than currently implemented. One could have ods dependencies installed, but not the Excel ones and vice versa. 
- When considering pandas could support more spreadsheet formats, does it make sense to rename pandas/io/excel.py to spreadsheet.py, and correspondingly, read_excel to read_spreadsheet? I guess this is not easy since it a lot of stuff out there already counts on the excel naming scheme.

For the record, there is one message on the [mailing list](https://groups.google.com/forum/?fromgroups#!topic/pydata/Zv-BXJNsP2U), and this PR will fix issue #2311.

ods support depends on [ezodf](https://github.com/T0ha/ezodf), and issue #2311 mentions some of the alternatives.


Labels:
name: IO Data
description: IO issues that don't fit into a more specific label
name: IO Excel
description: read_excel, to_excel

Comments:
As long as the tests are not implemented, you can quickly get into the action as follows:

``` python
import pandas as pd
pd.read_excel('pandas/io/tests/data/test.ods')
pd.read_excel('pandas/io/tests/data/test2.ods')
pd.read_excel('pandas/io/tests/data/test3.ods')
pd.read_excel('pandas/io/tests/data/test_converters.ods')
pd.read_excel('pandas/io/tests/data/test_types.ods')
pd.read_excel('pandas/io/tests/data/test_types2.ods')
```

@davidovitch Is there a good reason why you need this functionality in `pd.read_excel`? To me, the natural choice would be a new function  `pd.read_ods`. If it's just about duplicated code/docstrings, we have ways to work around that.

`pandas.io.spreadsheet` is certainly an option, but we'll need a shim to preserve the `pandas.io.excel` namespace for backwards compatibility. It would probably be easier to make a separate `pandas.io.ods` module and import any shared logic (ods/excel) another file.

For similar but not quite identical tests, my usual approach is to use multiple inheritance:  http://stackoverflow.com/a/1323554/809705

@davidovitch well, this is using a lot of duplicated code.

You can refactor to have the `engine` parameter decide how to open the file as well (rather than just using `ExcelFile`).

I don't think adding to the namespace is useful (e.g. `read_spreadsheet/ods`. This should work like `pd.read_excel(......, engine='ods')`.  Even better would be if those formats have a specific extension / format which can be determined at run-time? which could then just automatically set the engine

@jreback, I've refactored it as suggested: the `engine` parameter can now also be set to `ezodf` when using ods files. Everything is now under `ExcelFile`, which has 3 additional private methods to deal with ods files (`_parse_ods`, `_parse_datetime`, `_print_ods_cellinfo`).

The entire logic to derive at run-time the file type is extended and lives in `ExcelFile.__init__`. I don't think what I have done here is a very elegant approach though, but at least it should work in the same way for MS Excel files. If another spreadsheet file reader has to be added (gnumeric for example), the current approach probably has to be reconsidered. Maybe the `ExcelWriter` approach could work for the reader as well. But that will require more work from my part, and maybe the current approach would suffice for now?

Only the last reader test fails at this stage, and I have some work left in figuring out why it goes wrong.

The test that currently still fails also revealed a small typo in `pandas/io/parser.py`: davidovitch/pandas@49245c43ce38df18b2f3a00feced14654d9c0def

The ods writer is still on the todo list.

also need to add ezodf to the build process. This is not available on conda. But can pip install it. I can fix that so this will tests correctly.

Also would need an update to the install.rst docs (as this is now an alternative to xlrd).

So, after a long pause I am trying to get this going again. I just did a rebase with upstream and pushed my latest changes to my remote clone here. However, I think I did something not entirely correct because now I can see some other commits appearing in this pull request... 

@jreback, i have made an attempt to make it less messy, but I have to admit I wasn't completely sure I understood your suggestions correctly. I guess you'll let me know if this is not exactly right :-)

There is now another test failing, so this is not over yet. Anyway, thanks for reviewing this. It has been an interesting and learnfull ride so far.

@davidovitch ok, first order of biz is to rebase this. Then can see what the changes.

@jreback, I think git history is fixed now. All but the most recent commit were doubled previously. I have removed those commits with a rebase, followed by a push origin master -f to here.

@jreback, is this looking better? Regarding the tests for the readers (ExcelReaderTests), I added a method to determine which readers are installed, and only test for those. This is different then before where a test would skip if only one out of 3 readers was not installed. I think it looks less compact in the code, but should result in better test coverage when not all the spreadsheet readers are installed.
EDIT: On second thought, I think this helps maintainability, and it should be easier to add another reader and create tests for it.

@shoyer, @jreback I would like to bring this PR back to life. Is any one of you interested in another round of reviews?

Sure, let us know when you're ready

On Wed, Apr 15, 2015 at 9:32 AM, David Verelst notifications@github.com
wrote:

> ## @shoyer, @jreback I would like to bring this PR back to life. Is any one of you interested in another round of reviews?
> 
> Reply to this email directly or view it on GitHub:
> https://github.com/pydata/pandas/pull/9070#issuecomment-93477813

@shoyer, @jreback: I've cleaned it up a bit more. Personally, I believe this PR has come along way, but it is far from being perfect:
- No writer capabilities yet, and I think that should be another PR
- No tests yet with reading an ods file from a URL
- Not all file type determination logic is out of the `ExcelFile.__init__()` class. I think it would be more elegant if BaseFile and its subclasses (`EZODFFile` and `XLRDFile`) should contain the file format/library dependent logic.
- `ExcelFIle` still contains `ezodf` and `xlrd` dependent methods: `_parse_ods`, `_parse_datetime()`, and `_print_ods_cellinfo()` for `ezodf` and `_parse_excel()` for `xlrd`. I would assume it to be more elegant if those somehow could live in the respective `BaseFile` subclasses `EZODFile` and `XLRDFile`
- A few days ago I bumped into [pyexcel](https://github.com/chfw/pyexcel): a library with a single API to read all the different spreadsheet formats. It depends on the same libraries pandas is using for reading/writing of the different formats, and I am wondering if the work I am doing here makes any sense if, instead of implementing this for all the different readers/writers out there, we could focus on supporting one library that handles all the format dependent logic/interpretation for us. I also mention this in issue #2311. Any thoughts on this?

@davidovitch seems that `pyexcel` uses lots of different libraries (well plugins), and thus to add `ods` support you would need: `pyexcel, pyexcel-io, texttable, pyexcel-ods, pyexcel-ods3` is that accurate? if that is the case then too many deps.

@jreback you are right: `pyexcel` uses a plugin infrastructure. The plugins itself are very small (the actual `pyexcel-ods` code doing stuff is only 250 lines approximately). Since `pyexcel` uses a new BSD license (I am not a lawyer, but new BSD and BSD seem to be compatible), it seems we could also just incorporate `pyexcel` + plugins into `pandas` instead.

An alternative strategy could be to see if the `pyexcel` maintainer is open for maintaining/supporting a version that includes all the plugins, but one that fails graciously when the required readers are not installed (as is done now in `pandas`).

Either way, I don't have any strong feelings regarding the current approach or something different like `pyexcel`. After all, the code that is specific to a certain reader is limited, and writing it specifically with `pandas` in mind might be more suitable for certain edge cases. 

Difficult to say, but migrating to `pyexcel` might be more work compared to implementing the missing features into `pandas`.

I have rebased my work again, and squashed a lot of intermediate steps into the `ENH` commit. I hope this will help the review process.

I re-factored the tests so the structure is consistent with how the Excel writer tests are implemented. If I got it right this should be address @jreback comments. Previously, not all tests where executed for all the 3 different Excel formats (because not all files where available in all 3 formats. Now all test spreadsheets are available in the 3 formats (`xls`, `xlsx`, `xlsm`)

Although I agree with  @shoyer that using `None` instead of `type(None)` for `io_class` seems more elegant at first sight, `None` will fail when using `isinstance(object, None)`. Since at some places I need to test if a certain object is possibly something else then None, I kind of assume it is nicer to have:

``` python
if isinstance(object, a_type_that_can_be_NoneType):
    blah
```

over 

``` python
if object is not None and isinstance(object, a_type_that_cannot_be_NoneType):
    blah
```

I will squash and rebase in a few days, but I will retain the re-factoring of the reader tests in a separate commit.

using type(None) is not idiomatic at all
pls change this

Sure.

Is there any way of avoiding something like this instead?

``` Python
if self.io_class is None:
    return False
elif isinstance(io, self.io_class):
    return True
else:
    return False
```

I can see how `type(None)` violate Python idioms; I was trying to be consistent on the data type of io_class: It can be either `None`, or a type (`xlrd.Book` or `ezodf.document.PackagedDocument`). But maybe that doesn't matter at all :-)

return isinstance(io, self.io_class)

but that will fail if `self.io_class` is `None`... or am I seeing ghosts?

nope

That's weird, what I get is:

```
>>> isinstance(True, True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: isinstance() arg 2 must be a type or tuple of types
>>> isinstance(True, type(True))
True
>>> 
```

why would io_class be None

or just do

return io is None or isinstnace(io, io_class)

Because if the specific reader is not installed, it can be None. 
Yeah, that's way more compact :-) but the same principle still applies.

@jreback @shoyer: thanks again for your comments! Let me know if you prefer a fresh squash/rebase before having another look at this.

@shoyer @jreback and once more thanks for reviewing (hope this PR is not too boring for you guys...). I have tried to address all you comments.

I am not sure I see how to get around @jreback last suggestion: 

> why don't you make supported_readers a cached property which returns this list. This should return readers which don't raise import errors.
> this can be encapsulated in a method for each reader, e.g. in the xlrd, ezodf sub-classes, e.g. something like then the loop becomes
> 
> ``` python
> for reader in self.supported_readers:
>     try:
>           return reader.create_engine(io, engine=engine):
>     except (NotImplementedError):
>          pass
> raise ValueError("no supported readers for this input")
> ```

This approach would require another mechanism to throw and `ImportError` when the reader corresponding the input is supported but not installed. So I am not sure I fully understand how to get the last suggestion implemented properly.

Don't get me wrong, I am getting a lot out of your comments and suggestions, and I don't want to give up just yet (I would love to see pandas read my ods spreadsheets), but I would like to ask your opinion on how this is going as this is my first "significant" PR (I am not educated as a coder). Do you think the PR is going in the right direction? Are the "improvements" I am making getting the PR to an acceptable level? If I am not able to clear the quality bar, I would rather have someone more capable and experienced take this over instead. It would be a shame if am just wasting your time over a PR that is not going anywhere.

Just to let you know, I'm watching this thread and waiting eagerly for this feature. =)

I'm on my phone now, but if I recall correctly the code here was indeed on the right track. The key thing is that it's well tested -- clean code can come later.

On Sun, Jul 26, 2015 at 3:33 AM, David Verelst notifications@github.com
wrote:

> @shoyer @jreback and once more thanks for reviewing (hope this PR is not too boring for you guys...). I have tried to address all you comments.
> I am not sure I see how to get around @jreback last suggestion: 
> 
> > why don't you make supported_readers a cached property which returns this list. This should return readers which don't raise import errors.
> > this can be encapsulated in a method for each reader, e.g. in the xlrd, ezodf sub-classes, e.g. something like then the loop becomes
> > 
> > ``` python
> > for reader in self.supported_readers:
> >     try:
> >           return reader.create_engine(io, engine=engine):
> >     except (NotImplementedError):
> >          pass
> > raise ValueError("no supported readers for this input")
> > ```
> > 
> > This approach would require another mechanism to throw and `ImportError` when the reader corresponding the input is supported but not installed. So I am not sure I fully understand how to get the last suggestion implemented properly.
> > 
> > ## Don't get me wrong, I am getting a lot out of your comments and suggestions, and I don't want to give up just yet (I would love to see pandas read my ods spreadsheets), but I would like to ask your opinion on how this is going as this is my first "significant" PR (I am not educated as a coder). Do you think the PR is going in the right direction? Are the "improvements" I am making getting the PR to an acceptable level? If I am not able to clear the quality bar, I would rather have someone more capable and experienced take this over instead. It would be a shame if am just wasting your time over a PR that is not going anywhere.
> > 
> > Reply to this email directly or view it on GitHub:
> > https://github.com/pydata/pandas/pull/9070#issuecomment-124967579

@davidovitch I'll have a look tomorrow
but the point is tests are key - and the code should be pretty idiomatic
I have found that it is best to put in pretty tight code on the first pass - yes things can and do get revised afterwards - but that is harder esp if the original authors are not around (which can be frequent) and we already have enough to do w/o auditing all parts of the code