refactor: simplify da type checking by bwanglzu · Pull Request #224 · docarray/docarray

bwanglzu · 2022-03-23T21:44:07Z

DocumentArray is MutableSequence -> Sequence -> Iterable.

iter(da) is Generator -> Iterator -> Iterable.

chain(da1, da2) is both Iterable and Iterator.

Maybe we only need to check 1 type

from docarray import DocumentArray, Document

from typing import Generator, Iterator, Sequence, Iterable

da = DocumentArray([Document(), Document(), Document()])

print(isinstance(da, DocumentArray))
print(isinstance(da, Sequence))
print(isinstance(da, Iterable))


my_iter = iter(da)

print(isinstance(my_iter, Iterator))
print(isinstance(my_iter, Generator))

from itertools import chain

da1 = DocumentArray([Document()])
da2 = DocumentArray([Document(), Document()])

# chaining da1 and da2 numbers
da = chain(da1, da2)
print(isinstance(da, Iterator))
print(isinstance(da, Iterable))
print(type(da))

codecov · 2022-03-23T21:48:52Z

Codecov Report

Merging #224 (0a89610) into main (5a8544a) will increase coverage by 4.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #224      +/-   ##
==========================================
+ Coverage   81.18%   85.28%   +4.09%     
==========================================
  Files         123      123              
  Lines        5677     5667      -10     
==========================================
+ Hits         4609     4833     +224     
+ Misses       1068      834     -234

Flag	Coverage Δ
docarray	`85.28% <100.00%> (+4.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
docarray/array/storage/memory/backend.py	`95.23% <ø> (-0.22%)`	⬇️
docarray/array/storage/memory/getsetdel.py	`100.00% <ø> (ø)`
docarray/array/storage/weaviate/getsetdel.py	`100.00% <ø> (ø)`
docarray/array/chunk.py	`88.00% <100.00%> (-0.89%)`	⬇️
docarray/array/match.py	`86.95% <100.00%> (-1.05%)`	⬇️
docarray/array/storage/annlite/backend.py	`95.00% <100.00%> (-0.09%)`	⬇️
docarray/array/storage/qdrant/backend.py	`96.73% <100.00%> (-0.04%)`	⬇️
docarray/array/storage/sqlite/backend.py	`92.20% <100.00%> (-0.10%)`	⬇️
docarray/array/storage/weaviate/backend.py	`87.17% <100.00%> (-0.22%)`	⬇️
docarray/document/mixins/_property.py	`85.80% <0.00%> (+4.51%)`	⬆️
... and 7 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 5a8544a...0a89610. Read the comment docs.

hanxiao · 2022-03-24T06:33:38Z

@alaeddine-13 can you review it

alaeddine-13

Maybe it's clearer to just check for Iterable

alaeddine-13 · 2022-03-24T08:08:24Z

-            )
-            and self._ref_doc is not None
-        ):
+        if isinstance(docs, (Iterable, Iterator)) and self._ref_doc is not None:


Iterator is also a subclass of Iterable so we can just keep Iterable here

Suggested change

if isinstance(docs, (Iterable, Iterator)) and self._ref_doc is not None:

if isinstance(docs, Iterable) and self._ref_doc is not None:

alaeddine-13 · 2022-03-24T08:08:46Z

-            )
-            and self._ref_doc is not None
-        ):
+        if isinstance(docs, (Iterable, Iterator)) and self._ref_doc is not None:


Suggested change

if isinstance(docs, (Iterable, Iterator)) and self._ref_doc is not None:

if isinstance(docs, Iterable) and self._ref_doc is not None:

alaeddine-13 · 2022-03-24T08:09:46Z

-        if isinstance(
-            _docs, (DocumentArray, Sequence, Generator, Iterator, itertools.chain)
-        ):
+        if isinstance(_docs, (Iterable, Iterator)):


Suggested change

if isinstance(_docs, (Iterable, Iterator)):

if isinstance(_docs, Iterable):

alaeddine-13 · 2022-03-24T08:10:23Z

        elif isinstance(
            _docs,
-            (DocumentArray, Sequence, Generator, Iterator, itertools.chain),
+            (Iterable, Iterator),


just Iterable

alaeddine-13 · 2022-03-24T08:12:56Z

-        if isinstance(
-            docs, (DocumentArray, Sequence, Generator, Iterator, itertools.chain)
-        ):
+        if isinstance(docs, (Iterable, Iterator)):


Suggested change

if isinstance(docs, (Iterable, Iterator)):

if isinstance(docs, Iterable):

alaeddine-13 · 2022-03-24T08:13:16Z

-        elif isinstance(
-            _docs, (DocumentArray, Sequence, Generator, Iterator, itertools.chain)
-        ):
+        elif isinstance(_docs, (Iterable, Iterator)):


Suggested change

elif isinstance(_docs, (Iterable, Iterator)):

elif isinstance(_docs, Iterable):

alaeddine-13 · 2022-03-24T08:13:29Z

-        elif isinstance(
-            _docs, (DocumentArray, Sequence, Generator, Iterator, itertools.chain)
-        ):
+        elif isinstance(_docs, (Iterable, Iterator)):


Suggested change

elif isinstance(_docs, (Iterable, Iterator)):

elif isinstance(_docs, Iterable):

alaeddine-13

LGTM

refactor: simplify da type checking

3380771

github-actions Bot added size/s area/core component/array labels Mar 23, 2022

hanxiao requested a review from alaeddine-13 March 24, 2022 06:33

alaeddine-13 reviewed Mar 24, 2022

View reviewed changes

bwanglzu and others added 2 commits March 24, 2022 09:25

refactor: remove iterator from type check

7f10cbb

fix: check if Iterable

0a89610

alaeddine-13 approved these changes Mar 24, 2022

View reviewed changes

bwanglzu merged commit c19f057 into main Mar 24, 2022

bwanglzu deleted the refactor-typechecking branch March 24, 2022 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: simplify da type checking#224

refactor: simplify da type checking#224
bwanglzu merged 3 commits into
mainfrom
refactor-typechecking

bwanglzu commented Mar 23, 2022 •

edited

Loading

Uh oh!

codecov Bot commented Mar 23, 2022 •

edited

Loading

Uh oh!

hanxiao commented Mar 24, 2022

Uh oh!

alaeddine-13 left a comment

Uh oh!

alaeddine-13 Mar 24, 2022

Uh oh!

alaeddine-13 Mar 24, 2022

Uh oh!

alaeddine-13 Mar 24, 2022

Uh oh!

alaeddine-13 Mar 24, 2022

Uh oh!

alaeddine-13 Mar 24, 2022

Uh oh!

alaeddine-13 Mar 24, 2022

Uh oh!

alaeddine-13 Mar 24, 2022

Uh oh!

alaeddine-13 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if isinstance(docs, (Iterable, Iterator)) and self._ref_doc is not None:
	if isinstance(docs, Iterable) and self._ref_doc is not None:

	if isinstance(_docs, (Iterable, Iterator)):
	if isinstance(_docs, Iterable):

	if isinstance(docs, (Iterable, Iterator)):
	if isinstance(docs, Iterable):

	elif isinstance(_docs, (Iterable, Iterator)):
	elif isinstance(_docs, Iterable):

Conversation

bwanglzu commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hanxiao commented Mar 24, 2022

Uh oh!

alaeddine-13 left a comment

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bwanglzu commented Mar 23, 2022 •

edited

Loading

codecov Bot commented Mar 23, 2022 •

edited

Loading