replace arg 'transpose' by 'wide' in to_csv and to_excel (issues 371 + 549 + 575) #576

alixdamman · 2018-02-06T15:54:36Z

No description provided.

gdementen · 2018-02-07T09:11:20Z

this is related to #371 too

gdementen

Nice work. Just one thing worries me, it is the upgrade path if somebody used to_excel(transpose=True). Is it possible to somehow get the old behaviour? If so it should be mentioned how to get it in the release notes. If not, we will need to discuss this some more :)

gdementen · 2018-02-07T09:08:28Z

larray/core/array.py

        """
        Converts LArray into Pandas Series.

        Parameters
        ----------
+        name : str, optional


maybe add a changelog note about this?

gdementen · 2018-02-07T09:18:49Z

larray/core/array.py

+            replace NA values with na_rep. Defaults to ''.
+        wide : boolean, optional
+            Whether or not writing arrays in "wide" format. If True, arrays are exported with the last axis
+            represented horizontally. If False, arrays are exported in "narrow" format: all axes and also the values


"all axes and also the values are written as columns". This formulation is a bit weird => use something like "one column per axis plus one value column" instead?

gdementen · 2018-02-07T09:33:51Z

doc/source/changes/version_0_28.rst.inc

@@ -234,6 +233,50 @@ Miscellaneous improvements

  Closes :issue:`548`:

+* replaced argument `transpose` by `wide` in `to_csv` and `to_excel` methods. The new argument `wide` has


maybe split this in two separate sections:

to_csv(transpose -> wide), which does not need any example/explanation because it is a simple rename.

to_excel(transpose -> wide) which does need an explanation since the behaviour is slightly different

gdementen

my previous "general comment" (not tied to particular lines) still stands.

gdementen · 2018-02-14T08:31:40Z

Doh. I didn't say you needed to revert the change. I just wanted to discuss this some more. I am unsure it is worth keeping both arguments as it is likely to confuse users.

As far as I can tell, for 1D arrays, wide=False and transpose=True should give almost the same result (I think the only difference will be the name of the column), for 2D arrays, a.to_excel(transpose=True) should be equivalent to a.T.to_excel() and for >2D arrays, I don't know what transpose=True does :)

alixdamman · 2018-02-15T08:35:49Z

The following file contains examples using flags wide and transpose for 1D, 2D and 3D arrays:
test_excel.xlsx

Some users may find the transpose option useful for >=3D arrays. But in that case I personally find the 'pandas' output prettier (using arr3D.to_frame().transpose().to_excel(...)) than the 'LArray' output (arr3D.to_excel(..., transpose=True))

If we keep transpose, we need to prevent users to use wide and transpose in the same time.

gdementen · 2018-02-15T10:32:38Z

Thanks for this nice test file. Something like this should be in the "excel" part of the tutorial and MAYBE an abridged version should be in the changelog or at least something explaining the difference between the two arguments (not necessarily with examples)

gdementen · 2018-02-15T09:56:52Z

doc/source/changes/version_0_28.rst.inc

+
+* added argument `name` to `to_series` method allowing to set a name to the Pandas Series returned by the method:
+
+    >>> arr = ndtest((2, 3))


I wonder if having an example is worth the space for a little used feature.

gdementen · 2018-02-15T10:30:19Z

larray/core/array.py

@@ -5923,8 +5969,14 @@ def to_excel(self, filepath=None, sheet_name=None, position='A1', overwrite_file
        header : bool, optional
            Whether or not to write a header (axes names and labels). Defaults to True.
        transpose : bool, optional
-            Whether or not to transpose the resulting array. This can be used, for example, for writing one dimensional
-            arrays vertically. Defaults to False.
+            Whether or not to transpose the resulting array. Defaults to False.


This probably needs more explanation and/or an example. Maybe also saying that for 2D, it is equivalent to arr.transpose().to_excel()

gdementen

LGTM. Nice work, as usual...

…'transpose' of to_csv as 'wide' + added argument 'wide' to to_excel

…to set the name of the last column (i.e. the one containing the values) when exporting to csv/excel with wide=False

alixdamman requested a review from gdementen February 6, 2018 15:54

gdementen reviewed Feb 7, 2018

View reviewed changes

alixdamman requested a review from gdementen February 13, 2018 13:52

gdementen reviewed Feb 13, 2018

View reviewed changes

alixdamman requested a review from gdementen February 13, 2018 16:00

gdementen reviewed Feb 15, 2018

View reviewed changes

alixdamman requested a review from gdementen February 15, 2018 15:26

gdementen approved these changes Feb 15, 2018

View reviewed changes

alixdamman force-pushed the read_csv_undo_transpose_done_by_to_csv_549 branch from 3ab4818 to 090a1f8 Compare February 16, 2018 09:40

alixdamman changed the title ~~replace arg 'transpose' by 'wide' in to_csv and to_excel (issues 549 + 575)~~ replace arg 'transpose' by 'wide' in to_csv and to_excel (issues 371 + 549 + 575) Feb 16, 2018

alixdamman added 2 commits February 16, 2018 10:45

fix larray-project#575 and fix larray-project#371 : renamed argument …

85cb476

…'transpose' of to_csv as 'wide' + added argument 'wide' to to_excel

added argument 'name' to to_series

61e2bb9

alixdamman force-pushed the read_csv_undo_transpose_done_by_to_csv_549 branch from 090a1f8 to 10afd09 Compare February 16, 2018 09:45

fix larray-project#549 : added argument 'value_name' to to_csv/excel …

734e095

…to set the name of the last column (i.e. the one containing the values) when exporting to csv/excel with wide=False

alixdamman force-pushed the read_csv_undo_transpose_done_by_to_csv_549 branch from 10afd09 to 734e095 Compare February 16, 2018 10:47

alixdamman merged commit 21b7b8c into larray-project:master Feb 16, 2018

alixdamman deleted the read_csv_undo_transpose_done_by_to_csv_549 branch February 16, 2018 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace arg 'transpose' by 'wide' in to_csv and to_excel (issues 371 + 549 + 575) #576

replace arg 'transpose' by 'wide' in to_csv and to_excel (issues 371 + 549 + 575) #576

alixdamman commented Feb 6, 2018

gdementen commented Feb 7, 2018

gdementen left a comment

gdementen Feb 7, 2018

gdementen Feb 7, 2018

gdementen Feb 7, 2018

gdementen left a comment

gdementen commented Feb 14, 2018

alixdamman commented Feb 15, 2018 •

edited

Loading

gdementen commented Feb 15, 2018

gdementen Feb 15, 2018

gdementen Feb 15, 2018

gdementen left a comment

		@@ -234,6 +233,50 @@ Miscellaneous improvements

		Closes :issue:`548`:

		* replaced argument `transpose` by `wide` in `to_csv` and `to_excel` methods. The new argument `wide` has


		* added argument `name` to `to_series` method allowing to set a name to the Pandas Series returned by the method:

		>>> arr = ndtest((2, 3))

replace arg 'transpose' by 'wide' in to_csv and to_excel (issues 371 + 549 + 575) #576

replace arg 'transpose' by 'wide' in to_csv and to_excel (issues 371 + 549 + 575) #576

Conversation

alixdamman commented Feb 6, 2018

gdementen commented Feb 7, 2018

gdementen left a comment

Choose a reason for hiding this comment

gdementen Feb 7, 2018

Choose a reason for hiding this comment

gdementen Feb 7, 2018

Choose a reason for hiding this comment

gdementen Feb 7, 2018

Choose a reason for hiding this comment

gdementen left a comment

Choose a reason for hiding this comment

gdementen commented Feb 14, 2018

alixdamman commented Feb 15, 2018 • edited Loading

gdementen commented Feb 15, 2018

gdementen Feb 15, 2018

Choose a reason for hiding this comment

gdementen Feb 15, 2018

Choose a reason for hiding this comment

gdementen left a comment

Choose a reason for hiding this comment

alixdamman commented Feb 15, 2018 •

edited

Loading