fix #291 + #293 + #313 : Session.save (0D arrays + Excel + overwrite file by default) #312

alixdamman · 2017-06-13T08:23:59Z

until #153 is fixed.

gdementen

We need a test for #313 and changelog entries for both. A test for #291 would be nice but it's up to you given that it will be obsolete when we fix #153.

gdementen · 2017-06-13T12:33:54Z

larray/io/session.py

@@ -114,6 +115,10 @@ def dump_arrays(self, key_values, *args, **kwargs):
        display = kwargs.pop('display', False)
        self._open_for_write()
        for key, value in key_values:
+            if isinstance(value, ABCLArray) and value.ndim == 0:
+                if display:
+                    print('Cannot dump {}. Dumping 0D arrays is not supported currently.'.format(key))


currently not supported

alixdamman · 2017-06-13T14:30:25Z

Did you run pytest after commit 1286d67 ? Because you removed the larray method from Range class but not the call from getattr a few lines below.

gdementen · 2017-06-13T18:25:50Z

Oops, nice catch! I thought I did run the tests. Maybe I will need to add another one.

gdementen · 2017-06-14T06:54:22Z

doc/source/changes/version_0_24.rst.inc

@@ -57,3 +57,9 @@ Fixes
 * fixed the array() method on excel.Sheet returning float labels when int labels are expected.

 * fixed getting float data instead of int when converting an Excel Sheet or Range to an larray or numpy array.
+
+* fixed crash of Session.save method when it contains a 0D array.
+  0D arrays are skiped when saving a session (closes issue:`291`).


are now skipped

gdementen · 2017-06-14T07:17:14Z

doc/source/changes/version_0_24.rst.inc

+  0D arrays are skiped when saving a session (closes issue:`291`).
+
+* fixed crash of Session.save method when trying to save a session in an Excel that does not exist.
+  A new Excel file is created if it does not exist yet (closes issue:`313`).


isn't crash a too strong word for something which failed with a relatively nice error message? I would write something like:

fixed Session.save and Session.to_excel failing to create new Excel files (it only worked if the file already existed). Closes ...

gdementen · 2017-06-14T07:20:38Z

larray/tests/test_session.py

@@ -161,6 +162,8 @@ def test_xlsx_pandas_io(self):
    @pytest.mark.skipif(xw is None, reason="xlwings is not available")
    def test_xlsx_xlwings_io(self):
        fpath = abspath('test_session_xw.xlsx')
+        if os.path.isfile(fpath):


I think we also need an explicit test where the file already existed.

alixdamman · 2017-06-14T08:06:25Z

Do you want me to include issue #293 in this PR or another?

gdementen · 2017-06-14T08:07:33Z

as you prefer.

alixdamman · 2017-06-14T10:48:30Z

Not sure for issue #293

gdementen · 2017-06-14T11:48:49Z

larray/tests/test_session.py

+        s.save(fpath, engine='pandas_excel', overwrite=False)
+        s.load(fpath, engine='pandas_excel')
+        self.assertEqual(list(s.keys()), ['e', 'g', 'f'])
+        #assert_array_nan_equal(s['e'], self.e2)


why is this line commented?

gdementen · 2017-06-14T11:49:07Z

larray/tests/test_session.py

@@ -161,12 +178,21 @@ def test_xlsx_pandas_io(self):
    @pytest.mark.skipif(xw is None, reason="xlwings is not available")
    def test_xlsx_xlwings_io(self):
        fpath = abspath('test_session_xw.xlsx')
+        # test save when Excel does not exist


gdementen · 2017-06-14T11:52:05Z

larray/tests/test_session.py

+        # update an array (overwrite=False)
+        s['e'] = self.e2
+        s.save(fpath, overwrite=False)
+        s.load(fpath)


This test (and all similar tests below) is a bit misleading. It will pass even if the file is completely wiped out, because the arrays still exist in the session s and s.load(path) does not clear the session. Maybe we want to change that last point, but that would be for another PR/release.

gdementen · 2017-06-14T11:55:23Z

larray/core/session.py

            Dump using `engine`. Defaults to 'auto' (use default engine for
            the format guessed from the file extension).
+        overwrite: bool, optional
+            Whether or not to overwrite an existing file, if any. Only for non CSV files. 


Ignored for CSV files?

gdementen · 2017-06-14T11:58:24Z

doc/source/changes/version_0_24.rst.inc

@@ -18,6 +18,8 @@
     a0   0   1
     a1   2   3

+* added new boolean argument 'overwrite' to Session.save method. If True, overwrite existing file if any.


we should also mention other "save" methods: to_excel, etc. which are also impacted by this change.

I don't think it is clear enough that the default behaviour changed from overwrite=False to overwrite=True

gdementen · 2017-06-14T12:02:12Z

larray/io/session.py

@@ -164,7 +169,7 @@ def list(self):

    def _read_array(self, key, *args, **kwargs):
        df = self.handle.parse(key, *args, **kwargs)
-        return df_aslarray(df)
+        return df_aslarray(df, raw=True)


What's the relation with the rest? Is this an extra fix? If so it needs a test & changelog :)

This is an extra fix for an issue discovered when I tried to update the IO tests for Session ;)

In fact, this is not entirely correct. We should use: raw=index_col is None. But I admit the chance users ever hit this case is close to zero :)

forgot changelog, sorry.

…file if it does not exist.

…hod (defaults to True).

…l to df_aslarray function

gdementen · 2017-06-14T13:52:41Z

LGTM

alixdamman changed the title ~~fix #291 : excluding 0D arrays when saving a session.~~ fix #291 + #313 : excluding 0D arrays when saving a session + Session.save create new Excel file if not exist Jun 13, 2017

gdementen reviewed Jun 13, 2017

View reviewed changes

alixdamman force-pushed the misc2 branch from 1dc6552 to 6183b5e Compare June 13, 2017 14:25

alixdamman added the do not merge label Jun 13, 2017

gdementen reviewed Jun 14, 2017

View reviewed changes

alixdamman changed the title ~~fix #291 + #313 : excluding 0D arrays when saving a session + Session.save create new Excel file if not exist~~ fix #291 + #293 + #313 : Session.save (0D arrays + Excel + overwrite file by default) Jun 14, 2017

alixdamman force-pushed the misc2 branch from 473365c to 5f1274f Compare June 14, 2017 10:47

alixdamman removed the do not merge label Jun 14, 2017

alixdamman force-pushed the misc2 branch from 5f1274f to ec2f2c9 Compare June 14, 2017 11:37

gdementen reviewed Jun 14, 2017

View reviewed changes

alixdamman added 8 commits June 14, 2017 15:08

fix larray-project#291 : excluding 0D arrays when saving a session.

4f1f1d2

fix larray-project#313 : Session.save(file.xlsx) creates a new Excel …

475d1d1

…file if it does not exist.

updated IO tests for Session

246a3eb

fix larray-project#293 : added argument overwrite to Session.save met…

71e5c46

…hod (defaults to True).

updated IO tests for Session (included case overwrite=False)

03c0393

updated changelog 0.24

e4ecc22

fixed bug in PandasExcelHandler._read_array --> added raw=True in cal…

5ed3118

…l to df_aslarray function

updated IO tests for Session

f25bb7b

alixdamman force-pushed the misc2 branch from 1c574c3 to 4488b46 Compare June 14, 2017 13:08

alixdamman added 3 commits June 14, 2017 15:33

updated signature + doctstring of IO Session's methods.

1e705a3

updated changelog 0.24 (new argument overwrite ...)

7c91a98

updated changelog 0.24

be926e8

alixdamman force-pushed the misc2 branch from 4488b46 to be926e8 Compare June 14, 2017 13:33

gdementen merged commit 5962207 into larray-project:master Jun 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #291 + #293 + #313 : Session.save (0D arrays + Excel + overwrite file by default) #312

fix #291 + #293 + #313 : Session.save (0D arrays + Excel + overwrite file by default) #312

alixdamman commented Jun 13, 2017

gdementen left a comment

gdementen Jun 13, 2017

alixdamman commented Jun 13, 2017 •

edited

Loading

gdementen commented Jun 13, 2017

gdementen Jun 14, 2017

gdementen Jun 14, 2017

gdementen Jun 14, 2017

alixdamman commented Jun 14, 2017

gdementen commented Jun 14, 2017

alixdamman commented Jun 14, 2017

gdementen Jun 14, 2017

gdementen Jun 14, 2017

gdementen Jun 14, 2017

gdementen Jun 14, 2017

gdementen Jun 14, 2017

gdementen Jun 14, 2017

alixdamman Jun 14, 2017

gdementen Jun 14, 2017

alixdamman Jun 14, 2017

gdementen commented Jun 14, 2017

fix #291 + #293 + #313 : Session.save (0D arrays + Excel + overwrite file by default) #312

fix #291 + #293 + #313 : Session.save (0D arrays + Excel + overwrite file by default) #312

Conversation

alixdamman commented Jun 13, 2017

gdementen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alixdamman commented Jun 13, 2017 • edited Loading

gdementen commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alixdamman commented Jun 14, 2017

gdementen commented Jun 14, 2017

alixdamman commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdementen commented Jun 14, 2017

alixdamman commented Jun 13, 2017 •

edited

Loading