add a class that allows storing locations of errors #46

moenigin · 2023-08-15T18:13:42Z

add some typing and docstring

mjanusz

Thanks for adding the new class! I left a bunch of comments, mostly regarding code style and formatting issues.

ffn/utils/proofreading.py

mjanusz · 2023-08-20T17:03:23Z

Could you please squash all the commits within the PR into a single one, and also run pyink on the code, using the settings from https://github.com/google/ffn/blob/master/ffn/pyproject.toml ?

implements requested changes add a class that allows storing locations of errors - add some typing and docstring Update proofreading.py pass actionstate to store_error_location with different mode input directly & remove intermedeiary functions

moenigin · 2023-08-21T05:49:42Z

I hope what I did is what you were aiming for

mjanusz · 2023-08-21T22:17:25Z

Thanks! Unfortunately, it looks like it formatted everything with 4 spaces. One problem might have been that our pyproject.toml got accidentally moved to an incorrect location, so maybe that's why the tool didn't pick up the config. This is now fixed. Could you try syncing the repo and running pyink again? (or manually specify the settings that we use in the config file).

reformat with pyink from toml

correct remaining indent of 4 in docstring

moenigin · 2023-08-22T07:33:23Z

sorry for not checking this properly and all the hassle this created (was to happy about seeing pyink printing "all done" and trusted it did work according to toml). I noticed that the docstrings are not corrected to 2 indents by pyink. I did this manually now, but maybe pyink can do this on its own...

ffn/utils/proofreading.py

mjanusz · 2023-08-22T14:36:41Z

ffn/utils/proofreading.py

+      objects: A list of objects.
+      bad: A list of bad objects or markers.
+      seg_error_coordinates: A dictionary of error coordinates.
+      load_annotations: A flag to indicate if annotations should be loaded.


What's a use case where you would provide seg_error_coordinates, but set load_annotations to False?

A combination of laziness and generak flaws of the tool as is now. 1. If I pick up the review after a break I don't want to patch the final list of coordinates from different review session. 2. The annotation layer shows all annotations not only for the segment you are currently viewing. If in the last review session you already verfied that all locations you flagged in that session are correct, you may want to visualize only the novel locations.
Hope I am kind of making this clear here...

OK, so is the idea that when load_annotations == False, the seg_error_coordinates are just stored (and appended to as you annotate more), but not actually displayed?

Yes, that was my idea behind this because R. had already annotated >100 splits by the time we started this. Large numbers of annotations could be confusing. So if they are not needed because they are all verified from a previous revision round one can leave them out this way...
(Alternatively one could link the annotation to the errorneous segment of the todo list (requiring batch size to be one). By overriding update_segments one could only display the locations associated with a given batch of segments from the todo list. To view all marked location one could create a "show_all" function. However, as mentioned in the email thread I am not sure whether getting to split locations as implemented here is the most efficient way. Assembling segments belonging to one neurite by selection is usually faster. This would yield more coordinate pairs for a given split, but the would not necessarily be in close proximity to each other. If the latter is what you are after, that should be stated explicitly in the instructions.)

- include requested updates - fix bug for annotation selection

moenigin · 2023-08-22T19:32:28Z

thanks for the detailed inspection!

ffn/utils/proofreading.py

mjanusz · 2023-08-22T22:43:34Z

thanks for the detailed inspection!

Thanks for your patience with this. We're very close to having this completed now.

transform list of annotations id pair to frozenset

moenigin · 2023-08-23T16:02:09Z

thanks for the detailed inspection!

Thanks for your patience with this. We're very close to having this completed now.

I am learning a lot :-) E.g. that conventions are much more "local" than expected :-).

mjanusz · 2023-08-24T18:14:55Z

Could you please try running https://github.com/google/pytype on this code? We're getting a bunch of failures caused by the new annotations.

moenigin · 2023-08-24T21:04:21Z

Could you please try running https://github.com/google/pytype on this code? We're getting a bunch of failures caused by the new annotations.

This doesn't install on Windows. Unless there is an alternative to this I will not be able to check this for the next two weeks. Did the checks pass beforehand? Is there any hint to where the problem could be? I cannot view the details to this copybara thing either...

mjanusz · 2023-08-24T21:12:39Z

Here are the specific failures that are triggered:

File "third_party/py/ffn/utils/proofreading.py", line 65, in __init__: Built-in function len was called with the wrong arguments [wrong-arg-types]
         Expected: (obj: Sized)
  Actually passed: (obj: Optional[Iterable[Tuple[int, int, int]]])
  Attributes of protocol Sized are not implemented on None: __len__
File "third_party/py/ffn/utils/proofreading.py", line 71, in Base: Invalid type annotation 'list[str, Any]'  [invalid-annotation]
  list[_T] expected 1 parameter, got 2
File "third_party/py/ffn/utils/proofreading.py", line 115, in update_segments: unsupported operand type(s) for item retrieval: 'aa: str' and 'layer: str' [unsupported-operands]
  No attribute '__getitem__' on 'aa: str'
File "third_party/py/ffn/utils/proofreading.py", line 166, in list_segments: bad return type [bad-return-type]
           Expected: List[int]
  Actually returned: List[str]
File "third_party/py/ffn/utils/proofreading.py", line 316, in mark_bad: No attribute 'add' on list [attribute-error]
File "third_party/py/ffn/utils/proofreading.py", line 316, in mark_bad: No attribute 'add' on list [attribute-error]
  In Union[Any, list]
Called from (traceback):
  line 281, in <lambda>
File "third_party/py/ffn/utils/proofreading.py", line 318, in mark_bad: No attribute 'add' on list [attribute-error]
File "third_party/py/ffn/utils/proofreading.py", line 318, in mark_bad: No attribute 'add' on list [attribute-error]
  In Union[Any, list]
Called from (traceback):
  line 281, in <lambda>
File "third_party/py/ffn/utils/proofreading.py", line 330, in mark_removed_bad: unsupported operand type(s) for |: 'list' and 'new_bad: Set[int]' [unsupported-operands]
  No attribute '__or__' on 'list' or '__ror__' on 'new_bad: Set[int]'
File "third_party/py/ffn/utils/proofreading.py", line 355, in ObjectReviewStoreLocation: Type annotation for seg_error_coordinates does not match type of assignment [annotation-type-mismatch]
  Annotation: Optional[List[str]]
  Assignment: Dict[nothing, nothing]
File "third_party/py/ffn/utils/proofreading.py", line 359, in ObjectReviewStoreLocation: Invalid type annotation 'list[str, List[List[int]]]'  [invalid-annotation]
  list[_T] expected 1 parameter, got 2
File "third_party/py/ffn/utils/proofreading.py", line 373, in __init__: No attribute 'items' on List[str] [attribute-error]
  In Optional[List[str]]
File "third_party/py/ffn/utils/proofreading.py", line 418, in get_id: No attribute 'keys' on None [attribute-error]
  In Optional[List[str]]
File "third_party/py/ffn/utils/proofreading.py", line 418, in get_id: No attribute 'keys' on List[str] [attribute-error]
  In Optional[List[str]]
File "third_party/py/ffn/utils/proofreading.py", line 457, in store_error_location: No attribute 'update' on None [attribute-error]
  In Optional[List[str]]
File "third_party/py/ffn/utils/proofreading.py", line 457, in store_error_location: No attribute 'update' on List[str] [attribute-error]
  In Optional[List[str]]
File "third_party/py/ffn/utils/proofreading.py", line 526, in delete_location_from_annotation: unsupported operand type(s) for item deletion: None [unsupported-operands]
  No attribute '__delitem__' on None
File "third_party/py/ffn/utils/proofreading.py", line 526, in delete_location_from_annotation: unsupported operand type(s) for item deletion: List[str] and str [unsupported-operands]
  Function __delitem__ on List[str] expects Union[SupportsIndex, slice]
File "third_party/py/ffn/utils/proofreading.py", line 545, in delete_last_location: unsupported operand type(s) for item deletion: None [unsupported-operands]
  No attribute '__delitem__' on None
File "third_party/py/ffn/utils/proofreading.py", line 545, in delete_last_location: unsupported operand type(s) for item deletion: List[str] and str [unsupported-operands]
  Function __delitem__ on List[str] expects Union[SupportsIndex, slice]
File "third_party/py/ffn/utils/proofreading.py", line 686, in add_ccs: Function GraphUpdater.update_segments was called with the wrong arguments [wrong-arg-types]
         Expected: (self, segments: List[int], ...)
  Actually passed: (self, segments: set)

These are not new, we just got to this stage where this could be run. Any chance you could try WSL which the page you linked suggests would make it possible to run pytype? If this doesn't work, you could also try running another type checker like https://mypy-lang.org/

correct typing

mjanusz · 2023-09-12T13:59:58Z

Were you able to get type checking to work on your end?

I'm still seeing some failures with the latest change:

File "third_party/py/ffn/utils/proofreading.py", line 115, in update_segments: unsupported operand type(s) for item retrieval: 'aa: str' and 'layer: str' [unsupported-operands]
  No attribute '__getitem__' on 'aa: str'
File "third_party/py/ffn/utils/proofreading.py", line 166, in list_segments: bad return type [bad-return-type]
           Expected: List[int]
  Actually returned: List[str]
File "third_party/py/ffn/utils/proofreading.py", line 418, in get_id: No attribute 'keys' on None [attribute-error]
  In Optional[Dict[str, list]]
File "third_party/py/ffn/utils/proofreading.py", line 457, in store_error_location: No attribute 'update' on None [attribute-error]
  In Optional[Dict[str, list]]
File "third_party/py/ffn/utils/proofreading.py", line 526, in delete_location_from_annotation: unsupported operand type(s) for item deletion: None [unsupported-operands]
  No attribute '__delitem__' on None
File "third_party/py/ffn/utils/proofreading.py", line 544, in delete_last_location: Function reversed.__init__ was called with the wrong arguments [wrong-arg-types]
         Expected: (self, sequence: Reversible)
  Actually passed: (self, sequence: Optional[Dict[str, list]])
  Attributes of protocol Reversible[_T2] are not implemented on Dict[str, list]: __reversed__
File "third_party/py/ffn/utils/proofreading.py", line 545, in delete_last_location: unsupported operand type(s) for item deletion: None [unsupported-operands]
  No attribute '__delitem__' on None
File "third_party/py/ffn/utils/proofreading.py", line 686, in add_ccs: Function GraphUpdater.update_segments was called with the wrong arguments [wrong-arg-types]
         Expected: (self, segments: List[int], ...)
  Actually passed: (self, segments: set)

moenigin · 2023-09-12T15:38:23Z

Were you able to get type checking to work on your end?

I'm still seeing some failures with the latest change:

File "third_party/py/ffn/utils/proofreading.py", line 115, in update_segments: unsupported operand type(s) for item retrieval: 'aa: str' and 'layer: str' [unsupported-operands]
  No attribute '__getitem__' on 'aa: str'
File "third_party/py/ffn/utils/proofreading.py", line 166, in list_segments: bad return type [bad-return-type]
           Expected: List[int]
  Actually returned: List[str]
File "third_party/py/ffn/utils/proofreading.py", line 418, in get_id: No attribute 'keys' on None [attribute-error]
  In Optional[Dict[str, list]]
File "third_party/py/ffn/utils/proofreading.py", line 457, in store_error_location: No attribute 'update' on None [attribute-error]
  In Optional[Dict[str, list]]
File "third_party/py/ffn/utils/proofreading.py", line 526, in delete_location_from_annotation: unsupported operand type(s) for item deletion: None [unsupported-operands]
  No attribute '__delitem__' on None
File "third_party/py/ffn/utils/proofreading.py", line 544, in delete_last_location: Function reversed.__init__ was called with the wrong arguments [wrong-arg-types]
         Expected: (self, sequence: Reversible)
  Actually passed: (self, sequence: Optional[Dict[str, list]])
  Attributes of protocol Reversible[_T2] are not implemented on Dict[str, list]: __reversed__
File "third_party/py/ffn/utils/proofreading.py", line 545, in delete_last_location: unsupported operand type(s) for item deletion: None [unsupported-operands]
  No attribute '__delitem__' on None
File "third_party/py/ffn/utils/proofreading.py", line 686, in add_ccs: Function GraphUpdater.update_segments was called with the wrong arguments [wrong-arg-types]
         Expected: (self, segments: List[int], ...)
  Actually passed: (self, segments: set)

I have tried as far as possible without human support to get pytype running - it would not be recognized in WSL. I therefore used mypy. This did not run through error free but the errors that remained are non-overlapping with the ones you indicate:

proofreading.py:24: error: Skipping analyzing "networkx": module is installed, but missing library stubs or py.typed marker [import]
proofreading.py:24: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
proofreading.py:25: error: Skipping analyzing "neuroglancer": module is installed, but missing library stubs or py.typed marker [import]
proofreading.py:55: error: Need type annotation for "todo" (hint: "todo: List[] = ...") [var-annotated]
proofreading.py:67: error: Incompatible types in assignment (expression has type "None", variable has type "list[Sequence[int]]") [assignment]
proofreading.py:375: error: Need type annotation for "temp_coord_list" (hint: "temp_coord_list: List[] = ...") [var-annotated]
proofreading.py:509: error: Return value expected [return-value]
Found 6 errors in 1 file (checked 1 source file)

IIUC this is mypy not understanding some packages, complains about class attributes not being typed and does not understand None is returned in line509. I was hoping your typing tool would not crash there (apparantly the case!).

About the errors you get:
I can fix line 418, line 457, line 526, line 544, line 545 - had this fixed before but must have undone this somehow before committing. Sorry! I think the error in line 686 is likely fixed by cenverting line 95 to '''segments: Sequence[int],'''
However, the errors in line 115 and line 166 I don't understand. If self.todo is a list of dictionaries mapping lists of integer on layer (str) the typing should be correct. Do you see the error?

The type check you are doing is based on pytype? I can try to get help to get this running again but it may take me some time (human experts are scarce)

- fix/silence type hint errors (I hope) - fix error annotation count - fix error type assignment by only the second location

mjanusz · 2023-09-25T21:59:31Z

Thanks for the last round of changes! The latest version passes all our internal type checks, but I noticed that the annotation for the object list was actually not completely correct. I just pushed a commit that provides the correct annotations for that list -- could you please rebase on top of that and adjust the annotations where needed?

mjanusz · 2023-09-26T14:17:16Z

ffn/utils/proofreading.py

+  def update_segments(
+      self,
+      segments: Union[set[int], list[int]],
+      loc: Optional[Sequence[int]] = None,


pls use Optional[Point]

mjanusz · 2023-09-26T14:18:00Z

ffn/utils/proofreading.py

@@ -94,39 +117,55 @@ def update_segments(self, segments, loc=None, layer='seg'):
    else:
      l.equivalences.clear()
      for a in self.todo[self.index : self.index + self.batch]:
-        a = [aa[layer] for aa in a]
+        a = [cast(dict, aa)[layer] for aa in a]


Is the cast still needed now that self.todo is typed?

mjanusz · 2023-09-26T14:19:10Z

ffn/utils/proofreading.py

+      objects: Union[dict[str, int], Sequence[int]],
+      bad: set,
+      num_to_prefetch: int = 10,
+      locations: Optional[list[Sequence[int]]] = None,


pls use Point here

mjanusz · 2023-09-26T14:20:39Z

ffn/utils/proofreading.py

-  def __init__(self, objects, bad, num_to_prefetch=10, locations=None):
+  def __init__(
+      self,
+      objects: Union[dict[str, int], Sequence[int]],


pls use Iterable[ObjectItem]

mjanusz · 2023-09-26T14:21:00Z

ffn/utils/proofreading.py

+
+  def __init__(
+      self,
+      objects: list,


Iterable[ObjectItem]

mjanusz · 2023-09-26T14:21:31Z

ffn/utils/proofreading.py

+      self.make_point_annotation(coord, annotation_id)
+
+  def make_point_annotation(
+      self, coordinate: list[int], annotation_id: str


mjanusz · 2023-09-26T14:21:46Z

ffn/utils/proofreading.py

+      self.cur_error_type = None
+
+  def annotate_error_locations(
+      self, coordinates: list[list[int]], error_id: str


Iterable[Point]?

mjanusz · 2023-09-26T14:22:53Z

Looks great, just a few minor typing changes suggested above. Thanks!

mjanusz requested changes Aug 16, 2023

View reviewed changes

Update proofreading.py

54a34dd

implements requested changes add a class that allows storing locations of errors - add some typing and docstring Update proofreading.py pass actionstate to store_error_location with different mode input directly & remove intermedeiary functions

moenigin force-pushed the proofreading_updates branch from f787cce to 54a34dd Compare August 21, 2023 05:40

pyink run on proofreading.py

6bc2857

moenigin added 3 commits August 22, 2023 07:53

Merge remote-tracking branch 'upstream/master' into proofreading_updates

94c7229

Update proofreading.py

e5d48b9

reformat with pyink from toml

Update proofreading.py

813ce6d

correct remaining indent of 4 in docstring

mjanusz requested changes Aug 22, 2023

View reviewed changes

Update proofreading.py

b09792f

- include requested updates - fix bug for annotation selection

mjanusz reviewed Aug 22, 2023

View reviewed changes

ffn/utils/proofreading.py Outdated Show resolved Hide resolved

Update proofreading.py

d3cfa96

transform list of annotations id pair to frozenset

Update proofreading.py

5f7c55e

correct typing

mjanusz self-assigned this Sep 12, 2023

several fixes

45f2090

- fix/silence type hint errors (I hope) - fix error annotation count - fix error type assignment by only the second location

Merge remote-tracking branch 'upstream/master' into proofreading_updates

448f18a

mjanusz reviewed Sep 26, 2023

View reviewed changes

ffn/utils/proofreading.py

def __init__(

self,

objects: list,

Copy link

Collaborator

mjanusz Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterable[ObjectItem]

mjanusz reviewed Sep 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a class that allows storing locations of errors #46

add a class that allows storing locations of errors #46

moenigin commented Aug 15, 2023

mjanusz left a comment

mjanusz commented Aug 20, 2023

moenigin commented Aug 21, 2023

mjanusz commented Aug 21, 2023

moenigin commented Aug 22, 2023

mjanusz Aug 22, 2023

moenigin Aug 22, 2023

mjanusz Aug 22, 2023

moenigin Aug 23, 2023

moenigin commented Aug 22, 2023

mjanusz commented Aug 22, 2023

moenigin commented Aug 23, 2023

mjanusz commented Aug 24, 2023

moenigin commented Aug 24, 2023

mjanusz commented Aug 24, 2023

mjanusz commented Sep 12, 2023

moenigin commented Sep 12, 2023

mjanusz commented Sep 25, 2023

mjanusz Sep 26, 2023

mjanusz Sep 26, 2023

mjanusz Sep 26, 2023

mjanusz Sep 26, 2023

mjanusz Sep 26, 2023

mjanusz Sep 26, 2023

mjanusz Sep 26, 2023

mjanusz commented Sep 26, 2023

add a class that allows storing locations of errors #46

Are you sure you want to change the base?

add a class that allows storing locations of errors #46

Conversation

moenigin commented Aug 15, 2023

mjanusz left a comment

Choose a reason for hiding this comment

mjanusz commented Aug 20, 2023

moenigin commented Aug 21, 2023

mjanusz commented Aug 21, 2023

moenigin commented Aug 22, 2023

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

moenigin commented Aug 22, 2023

mjanusz commented Aug 22, 2023

moenigin commented Aug 23, 2023

mjanusz commented Aug 24, 2023

moenigin commented Aug 24, 2023

mjanusz commented Aug 24, 2023

mjanusz commented Sep 12, 2023

moenigin commented Sep 12, 2023

mjanusz commented Sep 25, 2023

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

mjanusz commented Sep 26, 2023