-
Notifications
You must be signed in to change notification settings - Fork 120
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
Remove 'xls' as by-default-supported file format #708
Conversation
Codecov Report
@@ Coverage Diff @@
## main #708 +/- ##
=======================================
- Coverage 94.8% 94.8% -0.1%
=======================================
Files 58 58
Lines 5899 5907 +8
=======================================
+ Hits 5598 5604 +6
- Misses 301 303 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@Rlamboll, in line with our earlier discussions, I tried to have the cake and eat it, too - reading xls is now properly supported (again), but only with a dedicated step for a user of installing the xlrd package - and with a test to prove that it works... Plus we let pandas handle file-types and engines natively, which is less overhead in our code and probably more forward-compatible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, just have a few concerns that the documentation (in some cases historic) may be confusing.
pyam/core.py
Outdated
iamc_index : bool, default False | ||
if True, use `['model', 'scenario', 'region', 'variable', 'unit']`; | ||
else, use all 'data' columns | ||
path : path-like or file-like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this description is unclear - it's presumably still accepting strings, as well as file objects or paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing... I looked at the pandas to_csv in more detail to be sure, and noticed that you can even use path=None
- now supported by pyam, and with a test.. (and I also improved the docs).
pyam/core.py
Outdated
File path or :class:`pathlib.Path`. | ||
iamc_index : bool, optional | ||
If True, use `['model', 'scenario', 'region', 'variable', 'unit']`; | ||
else, use all :meth:`dimensions`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to describe how you're using these variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a reference to the timeseries()
method which uses that argument. The documentation for the extra-columns feature needs an overhaul, but not as part of this PR.
pyam/core.py
Outdated
any valid string path, :class:`pathlib.Path` | ||
or :class:`pandas.ExcelWriter` | ||
excel_writer : path-like, file-like, or ExcelWriter object | ||
File path, :class:`pathlib.Path`, or existing :class:`pandas.ExcelWriter`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised.
if this is a string, use it as sheet name | ||
Name of sheet which will contain :meth:`timeseries` data. | ||
iamc_index : bool, optional | ||
If True, use `['model', 'scenario', 'region', 'variable', 'unit']`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See answer above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one small comment from my side.
Also without that would be good to merge from my side.
Very much appreciate the usage of the pandas defaults 👍.
Please confirm that this PR has done the following:
Name of contributors Added to AUTHORS.rstDescription of PR
This PR
When a user tries to import an xls-file, pandas will show this error message
Install xlrd manually and it will work fine - and there is a test to prove it.
Writing to xls is not support any more by pandas.
closes #706