fixed delay when switching to a big array (issue 93) #116

gdementen · 2017-11-10T10:56:08Z

one more big PR :)

also includes misc other fixes/improvements

you might want to review commits individually...

gdementen · 2017-11-10T11:11:36Z

oops, forgot to mention issue #93. I will reword/rebase one of the commits to include that.

… (issue larray-project#93) (so that we compute & redraw the new visible data only once)

also display the exception (even though it is currently unreadable)

…sue larray-project#93)

…ox for axes >= 10000 elements (issue larray-project#93)

…d out by isfinite

…es per change (issue larray-project#93)

gdementen · 2017-11-10T11:19:05Z

I flagged all relevant commits with the issue, the last one fixing it.

alixdamman · 2017-11-10T13:36:43Z

larray_editor/comparator.py

-        except:
-            self.array = LArray([np.nan])
+        except Exception as e:
+            self.array = LArray(str(e))


the idea is to display the error message in the array widget?

Yes, because I think that is the best place to display it. Having a dialog box popping up would be annoying. The error is currently unreadable because the cell is not automatically enlarged to fit the data, but since we should do that anyway, this will be solved later automatically. The message can still be copy-pasted in that case though. Maybe it would be better for now to use LArray("failed") instead, with a comment in the code saying to change it back to LArray(str(e)) when we automatically resize columns based on the data. I tested LArray("failed") earlier and it displayed nicely (but it was much less informative for me as to why it failed).

alixdamman · 2017-11-10T14:30:42Z

larray_editor/arraywidget.py

+            data = la.LArray([])
+        else:
+            data = la.aslarray(data)
+        axes = data.axes


IMO, the ArrayEditorWidget should not know what is the type of the data it gets (which is not the case for now...).
That's why I call self.data_adapter.set_data at the beginning of ArrayEditorWidget.set_data.
What I mean is that we will need to be careful once we will be ready to refactor the Adapter (and thus ArrayEditorWidget in the same time).
But for now, current changes are OK.

I don't think this is the right way to approach this. ArrayEditorWidget.set_data should either receive an adapter directly or instantiate one depending on the type of data it received. Having a single adapter instance and using set_data on it like we did cannot work because we will need a different adapter class for different data types.

Could you add a comment as a reminder?

Thanks for the comment.

alixdamman · 2017-11-10T14:32:57Z

larray_editor/arraywidget.py

+    # called by set_data and ArrayEditorWidget.accept_changes (this should not be the case IMO)
+    # two cases:
+    # * set_data should update both scientific and ndigits
+    # * toggling scientific checkbox should update only ndigits


So you want to split _update_digits_scientific in 2 methods?

Ideally yes, but this is tricky to do both efficiently and elegantly because there is some computation that needs to be shared for updating scientific and ndigits. It used to be split in more methods but I just remerged them for efficiency reasons. It is probably possible to have code that is both clean and efficient but I have not spent the time to reach that point and found the efficient version "acceptably clean".

alixdamman · 2017-11-10T14:36:14Z

larray_editor/arraywidget.py

+                # TODO: only do so if we would actually display more information
+                # 0.00001 can be displayed with 8 chars
+                # 1e-05
+                # would


would what?

no idea, this is an old comment :)

alixdamman · 2017-11-10T14:50:32Z

larray_editor/arrayadapter.py

    # - update_data (which sets new data but keeps current filter unchanged)
-    def set_data(self, data, bg_value=None, current_filter=None):
-        if data is None:
-            data = la.LArray([])


why moving these 2 lines (if data is None: ...) in ArrayEditorWidget?

Like I tried to explain in the commit comments: just because it makes the internal api simpler: you know what you get. In general, external API should accept as many useful things as possible, internal API should be as simple as possible.

alixdamman · 2017-11-21T08:29:45Z

Can I merge?

…t#93) * one of the goal was to make switching from one array to another as fast as possible by cutting down on the repeated calls (to various set_data and datamodel.reset()) * tightened what is accepted by each internal class. There is only one expected type for the data. External facing classes should still accept the same objects. * all internal classes which "hold" data are created without any data in __init__ but require a set_data before they can function. * data_model.reset_minmax needs to be called explicitly. * data_model.reset() needs to be called explicitly when "everything is done"

on the whole array but on a sample (fixes larray-project#93) This means we can miss the actual min/max of the displayed part, so it complexifies the code. Most of this awful code will go away after : * we invert how changes work (store old values instead of new values) * we get decent buffering (in that case the min/max should only be updated when "moving" the buffer.

gdementen · 2017-11-21T10:03:57Z

I'll squash the new commit I just added in another commit if you agree with that.

alixdamman · 2017-11-21T10:11:07Z

Please do

added example of big 1D array

e0dbcbd

gdementen changed the title ~~bigarrayspeed~~ fixed delay when switching to a big array (issue 93) Nov 10, 2017

gdementen added 12 commits November 10, 2017 12:16

when using the scrollwheel on filter comboboxes, only emit one signal…

5660a99

… (issue larray-project#93) (so that we compute & redraw the new visible data only once)

fixed diff_checkbox state being ignored when switching arrays

09f4927

fixed compare+diffonly when stack failed

49a5d11

also display the exception (even though it is currently unreadable)

fixed LArrayDataAdapter.get_xlabels being slow on large 1D arrays (is…

f5a0465

…sue larray-project#93)

prevent "filters" label from appearing for scalar (0D) arrays

713869a

made large 1D arrays display faster by not creating the filter combob…

244151c

…ox for axes >= 10000 elements (issue larray-project#93)

use min/max instead of nanmin/nanmax because nans are already filtere…

ef17c33

…d out by isfinite

select first row in compare(sessions)

d3901b1

fixed readonly being always False when editing arrays (doh!)

c535158

do not update_digits_scientific on accept_changes (it makes no sense)

4a4b2fd

made LinearGradient more foregiving (unsure it is a good idea)

049edec

reworked editor internals to avoid calling update methods several tim…

21ef4c8

…es per change (issue larray-project#93)

gdementen force-pushed the bigarrayspeed branch from af07b10 to 9e2b379 Compare November 10, 2017 11:18

gdementen requested a review from alixdamman November 10, 2017 12:06

gdementen added a commit to gdementen/larray that referenced this pull request Nov 10, 2017

added changelog for editor changes (larray-project/larray-editor#116)

dd0993e

alixdamman reviewed Nov 10, 2017

View reviewed changes

alixdamman approved these changes Nov 13, 2017

View reviewed changes

gdementen added a commit to larray-project/larray that referenced this pull request Nov 14, 2017

added changelog for editor changes (larray-project/larray-editor#116)

3ebcd92

gdementen added 3 commits November 21, 2017 10:50

cosmetic improvements to about dialog

77ede67

gdementen added 2 commits November 21, 2017 10:50

removed useless line (view, edit and compare are also in larray)

52eaf2b

added support for coloring object arrays (only use numeric values)

febd9f5

gdementen force-pushed the bigarrayspeed branch from 70d3ae8 to febd9f5 Compare November 21, 2017 10:12

gdementen merged commit 959fa6e into larray-project:master Nov 21, 2017

gdementen deleted the bigarrayspeed branch November 21, 2017 10:28

gdementen added a commit to larray-project/larray that referenced this pull request Aug 31, 2018

added changelog for editor changes (larray-project/larray-editor#116)

5b80cc3

fixed delay when switching to a big array (issue 93) #116

fixed delay when switching to a big array (issue 93) #116

Uh oh!

Conversation

gdementen commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gdementen commented Nov 10, 2017

Uh oh!

gdementen commented Nov 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdementen Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alixdamman commented Nov 21, 2017

Uh oh!

gdementen commented Nov 21, 2017

Uh oh!

alixdamman commented Nov 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gdementen commented Nov 10, 2017 •

edited

Loading

gdementen Nov 10, 2017 •

edited

Loading