Event handler refactor for image layer #1376

sofroniewn · 2020-06-20T20:10:14Z

Description

This PR is from the same branch as is PRed here shanaxel42#2 to #1040 directly. Due to my badness at git though that PR has all changes from master in it making it hard to review so I'm just going to make this one directly. It will also let me check if all tests pass on our CI.

Copying over my last comment from there:

The PR should be >99% backwards compatible in that no tests changed, I only added tests. Some private methods/ attributes changed. I don't think any public ones did, certainly not public ones that are part of our main API.

I will write more about this elsewhere, but the PR is a bit of a hodgepodge in places as we havn't fully moved over to the event handler for all attributes, all layers etc, or the dims models, but at least all functionality is working and it will be more clear how to extend things in the future.

There are a couple major points to discuss

Do we like this _transform_*_change method that I added which allows us to preprocess data on events inside the event handler before propagating to all listeners. It was basically essential for the name case I described above, but I can see it being useful elsewhere, see comment in #1040.
Do we want all our view like objects now only to make updates based on the value they receive in the _on_*_change method or can they continue to have access to the whole data model. As has been pointed out in a number of comments now this hybrid is a little odd, see our colormap handling and comment in #1040.
Do we need the Interface classes, and if so how should they look? I went the the checking if attr exists approach instead as sometimes I wanted to register a listener that only implemented an update method for one or two of the methods, see how I added listeners for the layerlist which has the _transform_name_method and the viewer which only has a few like _on_help_change. If each one of these had to have its own interface or if we allowed more partial interfaces then I'm not sure what's gained by having them at all, see comment in #1040.
We should revisit the changes to the Event class we pulled from vispy and make our changes backwards compatible or clearly document why we needed to make breaking changes there (for example the autoconnect stuff, which although we were setting before I don't think we were really making much use of). See comment in #1040 and comment in #1040.
We should think about how many event handlers we're going to need and where they should live and how they might talk with each other. I had to register the viewer and the layerlist to the layer event handler to get some of the stuff to work (name and active_layer) is this bad, is this ok? Will viewer get one and dims get one? Then maybe we are good?
We should think a bit about where more controller like stuff will live (thinking now about things like Layer models #1353) and the slicing of data. At least working that example out a bit before merge will probably be good.

There are also a couple minor points

A. We should add tests specifically for all the event handler stuff.
B. We should clean up all the doc strings, many are out of date now or wrong or missing.
C. We should think if we like having value everywhere in function signatures or if we want the name of the property etc.

We can always close this PR and work with the other ones, there are some great discussions happening there, or we can just press on with this one. I don't mind.

Probably next steps are in @napari/core-devs and @0x00b1 can weigh in on some of the big picture questions I mentioned above, and maybe @kne42 can go through and clean up some of the doc strings/ standardize function signatures/ add more comprehensive testing dedicated to the new functionality.

Type of change

Refactor (non-breaking change which fixes an issue)

Final checklist:

My PR is the minimum possible work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

codecov · 2020-06-20T20:16:26Z

Codecov Report

Merging #1376 into master will decrease coverage by 0.00%.
The diff coverage is 91.29%.

@@            Coverage Diff             @@
##           master    #1376      +/-   ##
==========================================
- Coverage   87.80%   87.79%   -0.01%     
==========================================
  Files         238      239       +1     
  Lines       22020    22048      +28     
==========================================
+ Hits        19334    19358      +24     
- Misses       2686     2690       +4

Impacted Files	Coverage Δ
napari/layers/surface/surface.py	`86.84% <0.00%> (-0.66%)`	⬇️
napari/layers/shapes/_shapes_mouse_bindings.py	`81.06% <50.00%> (ø)`
napari/_vispy/vispy_image_layer.py	`83.17% <68.88%> (-8.09%)`	⬇️
napari/_qt/qt_layerlist.py	`59.55% <76.31%> (-0.15%)`	⬇️
napari/utils/event.py	`67.93% <87.50%> (+3.71%)`	⬆️
napari/layers/shapes/shapes.py	`86.12% <90.00%> (-0.29%)`	⬇️
napari/_qt/layers/qt_image_base_layer.py	`94.06% <91.42%> (+0.20%)`	⬆️
napari/layers/base/base.py	`95.60% <98.24%> (-0.47%)`	⬇️
napari/_qt/layers/qt_base_layer.py	`100.00% <100.00%> (+9.30%)`	⬆️
napari/_qt/layers/qt_image_layer.py	`100.00% <100.00%> (+4.91%)`	⬆️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a483f16...10123ee. Read the comment docs.

sofroniewn · 2020-06-20T20:30:54Z

Interestingly this now has the failing windows test, which may previously have been fixed by #1347 and our mysterious scipy stats import. Not sure why this has reappeared here as that code should have been merged in ... The mystery there continues @tlambert03, but I wonder if this PR gives any clues ....

sofroniewn · 2020-06-22T02:57:12Z

Windows tests were fixed by #1377, all tests are now passing!!

jni · 2020-06-22T04:17:33Z

Do we like this _transform_*_change method that I added which allows us to preprocess data on events inside the event handler before propagating to all listeners. It was basically essential for the name case I described above, but I can see it being useful elsewhere, see comment in #1040.

I don't really. I think it would be nicer to write transform functions that get imported and used as needed.

Do we want all our view like objects now only to make updates based on the value they receive in the _on_*_change method or can they continue to have access to the whole data model. As has been pointed out in a number of comments now this hybrid is a little odd, see our colormap handling and comment in #1040.

I prefer things being based on the received value. I think this will make for more modular code and will make things like shrinking the number of object attributes much easier.

Do we need the Interface classes, and if so how should they look? I went the the checking if attr exists approach instead as sometimes I wanted to register a listener that only implemented an update method for one or two of the methods, see how I added listeners for the layerlist which has the _transform_name_method and the viewer which only has a few like _on_help_change. If each one of these had to have its own interface or if we allowed more partial interfaces then I'm not sure what's gained by having them at all, see comment in #1040.

Partial views are a very interesting use case, you're right. I still think it would be nice to document our models somehow with my dataclass "spec", so that people know what a complete view looks like.

I think there is a middle ground we need to find here. For example, one use case I was envisioning was a view that ignores interpolation changes, because they only support nearest neighbour interpolation. (For example, the thumbnails could be views of the layer!) However, it's good to know that the view is doing this ignoring, and have the ignoring be explicit somehow, rather than have it be implicit.

There also needs to be a check that all public attributes have a corresponding event.

How do you like this test idea: construct a "view" of the Image layer that is in fact another image layer. Randomly mutate the values of the original layer, serialize them both, and check that the serializations are identical.

We should revisit the changes to the Event class we pulled from vispy and make our changes backwards compatible or clearly document why we needed to make breaking changes there (for example the autoconnect stuff, which although we were setting before I don't think we were really making much use of). See comment in #1040 and comment in #1040.

Can you elaborate on this? I wasn't aware that we were diverging from VisPy, and I'd like any changes here to be somehow mirrored in a VisPy PR. Maybe that looks like separating VisPy events into BasicEvent and FancyEvent (aka Event), and we only use Basic. But I'd prefer that we don't diverge.

We should think about how many event handlers we're going to need and where they should live and how they might talk with each other. I had to register the viewer and the layerlist to the layer event handler to get some of the stuff to work (name and active_layer) is this bad, is this ok? Will viewer get one and dims get one? Then maybe we are good?

Why would it be bad to have multiple handlers? Isn't the idea to have one emitter (per model instance), many listeners?

We should think a bit about where more controller like stuff will live (thinking now about things like Layer models #1353) and the slicing of data. At least working that example out a bit before merge will probably be good.

I actually was hoping to keep that discussion separate and happening after this PR. They interact in lots of ways and trying to consider both changes simultaneously feels like too much to chew on. I think we can probably handle this first and then simplify the models — and having well defined interfaces as discussed in point 2 will only help with that. For example, in my dataclass model, that's not actually the full interface but rather ImageRenderingParametersInterface, to be fully verbose. So all those parameters might form a nice bundle of a class. The Image itself also has Data, for example.

sofroniewn · 2020-06-23T01:51:19Z

Had good chats with @shanaxel42 and @jni about this PR today, and there is good alignment. A couple key takeaways (numbering aligns with numbers above).

People agree the _transform_*_change methods aren't great, and it would be nice to see if we can figure out if we don't need them. It might necessitate adding a layerlist event handler to this PR, but that was going to happen eventually, so I'll investigate and post more.
People agree that getting to a world where we can drop the layer property from some of our view objects and just rely on the value and the function to the update is what we want. Some ways to approach this that were discussed include passing a dictionary for value with all the important things, but it was also raised that for many of the places that it isn't working right now we might actually be able to clean up the code to make it work, and this might make things much more modular and robust. What's nice about this change is that we can approach the refactor gradually doing it piece by piece.
We didn't resolve the Interfaces, right now I'm not using them everywhere, but they do have nice documentation properties. The main issue right now is that some of our "listeners" might not want to implement _on_*_change methods for all the events, so unclear if we just want to have methods that pass or use the getattr approach I have now. What's nice here is that we can take a gradual approach to changing this too.

I will try and add a layerlist handler remove the _transform_*_change method from this PR as a next step.

sofroniewn · 2020-06-23T04:50:37Z

So after more investigation the case of forcing the name to be unique in the layerlist remains tricky. It is a bit of a special case/ one off and so @jni and I are wondering @kne42 if maybe that can be solved with a mechanism entirely contained to the layerlist base class - i.e. the concept of a uniquely named list. The hard thing though is it will have to maintain unique names even as the user tries to update them. Need to think about this more, but it seems a shame to radically change the event handler system to accommodate this one special case where we need to inject some global state (the full list of layer names) into what is otherwise a pretty local operation (updating the layer name). Would love if @shanaxel42 has any more ideas on how to do this one too ...

jni · 2020-06-23T06:54:20Z

Yeah the order of events that makes sense to me is:

the layer attempts to change its name, and as far as it's concerned, it's succeeded
then the layer list, which is subscribed to layers changing their names, gets notified, checks against the global list, and resolves any conflicts.
it then goes back and changes the layer's name.
this triggers another event, but at this point, the layer list is happy and the loop stops.

For step 2, you need to know which layer changed its name. This can only work by knowing the event source or by emitting an (old_name, new_name) tuple rather than just the new name.

The only other alternative I see is that layers don't in themselves have names. Instead, only the layer list gives them names. Then we are sidestepping this whole dance and this will always be handled by the layer list.

tlambert03 · 2020-06-23T12:48:40Z

This can only work by knowing the event source or by emitting an (old_name, new_name) tuple rather than just the new name.

maybe this is already obvious (can't tell by the phrasing here), but just wanted to point out that the event object does have a source attribute that provides the object that emitted the event.

sofroniewn · 2020-06-23T14:50:18Z

To make that possible @jni we need to add a concept to the event handler of "priority" or "ordering" of the callbacks, as we need to make the layerlist a listener to the layer but make sure that it's callback executes last (otherwise even if it renames the layer during it's update there will be other updates that have yet to happen and when they do they will use the old name and things will get out of sync).

Also if the layerlist update happens last at that point we'll have two layers with the same name and know knowledge of which was which, and which had the old name as the update has already happened. Passing a tuple of (index, new_name) will solve that part. [EDIT: that won't work as the layer doesn't know it's index ...]

maybe this is already obvious (can't tell by the phrasing here), but just wanted to point out that the event object does have a source attribute that provides the object that emitted the event.

Indeed, but we've lost that by the time the _on_name_change(value) gets called inside the layerlist

[EDIT: I'm really struggling to see how we can make this use case possible without some major changes]

sofroniewn · 2020-06-23T15:02:27Z

I'm going to try a new approach which will use two events instead of one ....

jni · 2020-06-23T15:18:04Z

To make that possible @jni we need to add a concept to the event handler of "priority" or "ordering" of the callbacks, as we need to make the layerlist a listener to the layer but make sure that it's callback executes last (otherwise even if it renames the layer during it's update there will be other updates that have yet to happen and when they do they will use the old name and things will get out of sync).

I don’t think this matters. In step 3, the layer list updates the layer name. At that point, the layer eventhandler emits events with the new name, and of I’m not mistaken these will necessarily be processed by listeners after the original name change — so things will get in sync after just a few “clock cycles”. Am I missing something?

sofroniewn · 2020-06-23T18:33:39Z

I don’t think this matters. In step 3, the layer list updates the layer name. At that point, the layer eventhandler emits events with the new name, and of I’m not mistaken these will necessarily be processed by listeners after the original name change — so things will get in sync after just a few “clock cycles”. Am I missing something?

This is really about guaranteeing that step 1 is complete before this change happens, i.e. if the layer list event handler can make this change while the other event is still processing then it won't result in serial updates, it will result in a nested update that then gets overwritten. Certainly the implementation I tried suffered from this in practice.

If the layer widget emits name A then the layerlist hears it first (which may happen with no "priority"/ "ordering") and then changes it to A_1 then emits again then the layer hears it takes on A_1 and that event finishes, but then the original A from the layer widget come in and sets the layer name back to A!!!

The "two event approach" might work, though

jni

@sofroniewn very good! I've left mostly very minor comments, or requests for clarification. I would say the biggest concern is that I'm unsure again about unifying on value, as it makes the actual code more difficult to read when it's not a one-liner.

I also think a high priority for this refactor is to enshrine @shanaxel42's information flow diagram to the docs somewhere so that new contributors are comfortable with the napari eventhandler model.

I'll approve because I don't want to hold this up any longer, but stuff to think about. Everything else is just typos. =) Thanks to all involved!!!

jni · 2020-06-29T08:57:43Z

napari/_qt/layers/qt_base_layer.py

+        # Once EVH refactor is done, these can be moved to an initialization
+        # outside of this object
+        self._on_opacity_change(self.layer.opacity)
+        self._on_blending_change(self.layer.blending)


Can you explain this for me? Currently I found it confusing that this wasn't happening in this refactor. At any rate, I think at least the _on_*_change calls should get bundled with their respective box initialisations... (ie move _on_opacity_change to line 67 and on_blending_change to line 72)

Yeah, this is a good question. It's hard right now as we don't have an explicit concept of a controller which is I guess where these imports, initialization and wiring up would be done (as it would be the one place which knows about both model and view). We can't do it in the event handler itself as we import the event handler into the layer model directly so that we can edit the layer model without the views, so we need an additional concept here. I'll work on some architecture docs in a new PR and we can revisit this there.

As to the grouping, I was grouping the soon to be obsolete properties together to make easier to remove them later.

Ooo - ok, I have one idea - we could add it to the register_listener method of the event handler. It would be nice if we had a standardized way of figuring out what the event values should be and then re-emitting them. i.e. when you register the qt_image_layer we see that it has the _on_visible_change method and we know that the Image layer emits the visible event, and we know the current visible value lives at layer.visible so we call _on_visible_change(layer.visible) for the qt_image_layer?

We might be baking in a lot of assumptions there, but it would remove the need to a lot of initialization code throughout the repo. Curious @tlambert03 @jni @ziyangczi what you think of this idea?

napari/_qt/layers/qt_base_layer.py

jni · 2020-06-29T09:02:46Z

napari/_qt/layers/qt_image_base_layer.py

+        # Once EVH refactor is done, these can be moved to an initialization
+        # outside of this object


As above... I'd like a bit of clarification of when this is supposed to happen (ie what exactly has to be done), and maybe move the items to their respective sliders, although now that it's more things to change than two I see the value of grouping near-obsolete things together...

See discussion above

napari/_qt/layers/qt_image_base_layer.py

napari/_qt/layers/qt_image_layer.py

jni · 2020-06-29T14:33:27Z

napari/layers/points/points.py

@@ -451,7 +450,7 @@ def data(self, data: np.ndarray):
        elif len(data) > cur_npoints:
            # If there are now more points, add the size and colors of the
            # new ones
-            with self.events.set_data.blocker():
+            with self.events.slice_data.blocker():


Can you comment on why we need blockers here? I thought the whole point of the refactor was to not need blockers? Or is it because the refactor isn't actually done with the points layer?

Points refactor not done yet - hopefully this will go when it is!!!!

jni · 2020-06-29T14:34:23Z

napari/layers/vectors/vectors.py

@@ -243,7 +243,8 @@ def data(self, vectors: np.ndarray):
        self._displayed_stored = copy(self.dims.displayed)

        self._update_dims()
-        self.events.data()
+        self._update_editable()


What's editable exactly? I noticed @tlambert03 asking about it in another comment but to be honest the answer didn't really clarify anything for me...?

Editable is both a user setting and an attribute automatically determined that sets where things like adding points/ drawing with the paintbrush etc are allowed or not. We added both because someone might want to disable such functionality themselves AND because there are certain configurations like multiscale or 3D viewing where we don't support such interactivity. Definitely room for improvement here, but that's what the parameter is

napari/utils/event.py

jni · 2020-06-29T14:37:10Z

napari/utils/event.py

+                )
+        args = self.default_args.copy()
+        args.update(kwargs)
+        return self.event_class(**args)


Oof. This whole change makes me dislike the value kwarg even more. Perhaps the keyword can be the event name?

I actually don't mind value on the event as the one place where the payload is being carried. To me this makes a lot of sense and is fairly readable, but I see your comment about value inside the handlers and think we should change it there. @tlambert03 thoughts?

Sorry I’m on mobile at the moment and can’t see the full conversation. I don’t care what you call the argument in the handler... but I agree with @sofroniewn that the event payload should be standardized. I hated having to go find the event emitter to figure out the name of the event object attribute I’m supposed to be querying to retrieve the value. I also don’t mind this code block either if it lets us make connections easier.
@jni is your main objection the argument name in the handler? Or do you want the actual event object to have variable attribute names

napari/utils/event_handler.py

jni · 2020-06-29T14:50:23Z

yeah. I'm was a bit curious about the opposition to touching the EventEmitter object (and the recent reversion to make sure it matched the vispy code). why are we vendoring that code if we don't want to modify it?

I don't know about this specific instance, but in general the idea we've had with vendoring is to allow us to incorporate a quick bug or behaviour fix but always with the idea of contributing upstream and eventually removing the vendored copy. Whereas modifying entails a full divergence and I hate that — I want to play nicely with the ecosystem and reduce redundancy.

jni · 2020-06-29T14:50:53Z

(which would argue against the modifications to the event classes in this PR...)

jni · 2020-06-29T14:51:36Z

@sofroniewn please rename this PR to something less self-referential before merging! =)

Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

sofroniewn · 2020-06-29T17:05:07Z

@jni thanks for the review! Did you have any feedback on the decorator approach instead of _on_*change approach that @tlambert03 tried out here #1376 (comment) for identifying callbacks. It does strike me that if we need to have an object listening to multiple event handlers something like this might necessary otherwise we can't reuse the names of events.

There's some discussion happening in the comments about where we will initialization of our views. We need to think about that more, I feel like we might need a controller concept, see here #1376 (comment) as it would be the one place that knows about both models and viewers, but it is nuanced.

As to using value vs actual name like rendering in function signature, I'm fine going with actual name, but want to hear from @tlambert03 before making the change.

I'll work on an architecture doc in a new docs only PR

tlambert03 · 2020-06-29T17:12:30Z

FWIW, I’m extremely Luke warm on the decorator as it stands. I would support merging this and revisiting if needed (it’s mostly additions to this rather than replacements)

liu-ziyang · 2020-06-29T17:56:42Z

napari/_qt/layers/qt_base_layer.py

+            status=Event,
+        )
+
+        # When the EVH refactor #1376 is done we might not even need the layer


shall we use a TODO signature? haven't seen much in napari codebase, is it discouraged?

I'm open - thoughts @jni? @tlambert03?

liu-ziyang · 2020-06-29T17:57:13Z

napari/_qt/qt_layerlist.py

+            visible=Event,
+        )
+
+        # When the EVH refactor #1376 is done we might not even need the layer


see above for TODO

liu-ziyang · 2020-06-29T17:58:25Z

napari/utils/event_handler.py

+            return
+
+        # Update based on event value
+        for component in self.components_to_update:


we talked about this regarding having annotation-based detection vs name-based detection, wonder what others' opinion on this too

See discussion above and bellow this comment, and links in this comment for potential implementation #1376 (comment)

liu-ziyang · 2020-06-29T17:59:23Z

napari/utils/naming.py

+    if name not in names:
+        return name
+    else:
+        return force_unique_name(inc_name_count(name), names)


what if the forced name already exists? shall this method loop and check?

The looping and checking is happening recursively as the original function is called force_unique_name and then calling itself inside itself.

liu-ziyang · 2020-06-29T19:32:48Z

agreed with talley, let's merge this one and think more on the decorator. I still love to have a decorator based detection, but like talley said it is more complicated and introduced a lot of lines. (maybe a composite method where we name search and use a decorator for special cases?)

This reverts commit e43f730.

* Revert "Use qlistwidget for QtLayerList (#1391)" This reverts commit 33e7a74. * Revert "Event handler surface layer (#1396)" This reverts commit 5acd648. * Revert "Fix vispy volume colormap changing (#1402)" This reverts commit 9ea724b. * Revert "Event handler refactor for image layer (#1376)" This reverts commit e43f730. * partial return of qlayerlist * Revert "partial return of qlayerlist" This reverts commit 897c454.

Shannon Axelrod and others added 13 commits May 15, 2020 13:43

refactor with event handdler and interface

e32b785

fix bugs on startup

d2b3282

more bug fixes

5242b1c

fix layer widget

f3a6a35

more fixes

423ee43

more fixes

6cf1eb2

even more fixes

fc11d61

getattr method

4ba3348

get layer naming working

a9f182e

fix data update

74cf24c

Merge branch 'master' into event_handler_njs

fa80632

fix attenuation

6b22ce3

update doc string

5230b2c

sofroniewn added the refactor Refactoring code base label Jun 20, 2020

sofroniewn added this to the 0.3.6 milestone Jun 20, 2020

sofroniewn requested a review from a team June 20, 2020 20:10

Merge branch 'master' into event_handler_njs

af8c908

drop transform

5ebe6cf

a bit more cleanup

4bed6e3

sofroniewn mentioned this pull request Jun 29, 2020

Use qlistwidget for QtLayerList #1391

Merged

5 tasks

jni approved these changes Jun 29, 2020

View reviewed changes

Apply suggestions from code review

e3fe2ab

Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

sofroniewn changed the title ~~Event handler continued!~~ Event handler refactor for image layer Jun 29, 2020

revert value

bfd262a

liu-ziyang reviewed Jun 29, 2020

View reviewed changes

sofroniewn added 2 commits June 29, 2020 15:46

more minor updates

454ba96

Merge branch 'master' into event_handler_njs

10123ee

sofroniewn mentioned this pull request Jun 30, 2020

Add EventHandler docs #1394

Closed

sofroniewn merged commit e43f730 into napari:master Jun 30, 2020

sofroniewn mentioned this pull request Jun 30, 2020

RFC: Refactor proposal! Adding event handlers to napari (TAKE 3) #1040

Closed

liu-ziyang mentioned this pull request Jun 30, 2020

Event handler surface layer #1396

Merged

5 tasks

This was referenced Jul 5, 2020

Colormap in 3D broken by Image Layer Event Handler #1401

Closed

Fix vispy volume colormap changing #1402

Merged

Cannot change interpolation mode or attenuation value with invisible layers #1398

Closed

This was referenced Jul 8, 2020

event handler refactor for vector layer #1408

Closed

add event handler for label layer #1410

Closed

sofroniewn mentioned this pull request Jul 9, 2020

fix grid mode #1412

Closed

jni mentioned this pull request Jul 9, 2020

Concerns about event handler patterns #1409

Closed

6 tasks

tlambert03 added a commit to tlambert03/napari that referenced this pull request Jul 10, 2020

Revert "Event handler refactor for image layer (napari#1376)"

6cba8f2

This reverts commit e43f730.

This was referenced Jul 12, 2020

Base EventedModel proposal #1436

Closed

Preserve tests from EVH revert #1452

Merged

		# Once EVH refactor is done, these can be moved to an initialization
		# outside of this object

Event handler refactor for image layer #1376

Event handler refactor for image layer #1376

Conversation

sofroniewn commented Jun 20, 2020 • edited

Description

Type of change

Final checklist:

codecov bot commented Jun 20, 2020 • edited

Codecov Report

sofroniewn commented Jun 20, 2020

sofroniewn commented Jun 22, 2020

jni commented Jun 22, 2020

sofroniewn commented Jun 23, 2020

sofroniewn commented Jun 23, 2020

jni commented Jun 23, 2020

tlambert03 commented Jun 23, 2020

sofroniewn commented Jun 23, 2020 • edited

sofroniewn commented Jun 23, 2020

jni commented Jun 23, 2020

sofroniewn commented Jun 23, 2020

jni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofroniewn Jun 29, 2020 • edited

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

Choose a reason for hiding this comment

jni commented Jun 29, 2020

jni commented Jun 29, 2020

jni commented Jun 29, 2020

sofroniewn commented Jun 29, 2020

tlambert03 commented Jun 29, 2020

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

liu-ziyang commented Jun 29, 2020

sofroniewn commented Jun 20, 2020 •

edited

codecov bot commented Jun 20, 2020 •

edited

sofroniewn commented Jun 23, 2020 •

edited

sofroniewn Jun 29, 2020 •

edited