DictOfNamedArrays: better repr, keys #485

matthiasdiener · 2024-02-19T21:18:04Z

Please squash

matthiasdiener · 2024-02-19T21:20:43Z

pytato/array.py

+    def keys(self) -> KeysView[str]:
+        return self._data.keys()


Not a super-impactful change, but printing keys is more convenient with this:

print(dona.keys())

before:

KeysView(DictOfNamedArrays({'_pt_out_0_0': Einsum(axes=(Axis(tags=FrozenOrderedSet({DiscretizationElementAxisTag()})), Axis(tags=FrozenOrderedSet({DiscretizationDOFAxisTag()}))), tags={FirstAxisIsElementsTag()}, access_descriptors=((EinsumElementwiseAxis(dim=0), EinsumElementwiseAxis(dim=1)), (EinsumElementwiseAxis(dim=1), EinsumReductionAxis(dim=0)), (EinsumElementwiseAxis(dim=0), EinsumReductionAxis(dim=0))), args=(IndexLambda(axes=(Axis(tags=FrozenOrderedSet({DiscretizationElementAxisTag()})), Axis(tags=FrozenOrderedSet({DiscretizationDOFAxisTag()}))), shape=(512, 1), dtype='float64', expr=Quotient(1.0, Subscript(Variable('_in1'), (Variable('_0'), Variable('_1')))), bindings={'_in1': DataWrapper(axes=(Axis(tags=FrozenOrderedSet({DiscretizationElementAxisTag()})), Axis(tags=FrozenOrderedSet({DiscretizationDOFAxisTag()}))), tags={PrefixNamed(prefix='area_el_vol')}, data=<arraycontext.impl.pyopencl.taggable_cl_array.TaggableCLArray object at 0x1666b2410>, _shape=(512, 1))}), DataWrapper(axes=(Axis(tags=frozenset({DiscretizationDOFAxisTag()})), Axis(tags=frozenset({DiscretizationDOFAxisTag()}))), tags={}, data=<arraycontext.impl.pyopencl.taggable_cl_array.TaggableCLArray object at 0x1666b3690>, _shape=(10, 10)), IndexLambda(axes=(Axis(tags=FrozenOrderedSet({DiscretizationElementAxisTag()})), Axis(tags=FrozenOrderedSet({DiscretizationDOFAxisTag()}))), shape=(512, 10), dtype='float64', expr=Sum((Subscript(Variable('_in0'), (Variable('_0'), Variable('_1'))), Subscript(Variable('_in1'), (Variable('_0'), Variable('_1'))))), bindings={'_in0': IndexLambda(axes=(Axis(tags=FrozenOrderedSet({DiscretizationElementAxisTag()})), Axis(tags=FrozenOrderedSet({DiscretizationDOFAxisTag()}))), shape=(512, 10), dtype='float64', expr=Product((-1.0, Subscript(Variable('_in1'), (Variable('_0'), Variable('_1'))))), bindings={'_in1': Einsum(axes=(Axis(tags=FrozenOrderedSet({DiscretizationElementAxisTag()})), Axis(tags=FrozenOrderedSet({DiscretizationDOFAxisTag()}))), tags={FirstAxisIsElementsTag()}, access_descriptors=((EinsumReductionAxis(dim=0), EinsumReductionAxis(dim=1), EinsumElementwiseAxis(dim=0), EinsumReductionAxis(dim=2)), (EinsumReductionAxis(dim=1), EinsumElementwiseAxis(dim=1), EinsumReductionAxis(dim=2)), (EinsumReductionAxis(dim=0), EinsumElementwiseAxis(dim=0), EinsumReductionAxis(dim=2))), args=((...), (...), (...)), redn_axis_to_redn_descr={EinsumReductionAxis(dim=0): ReductionDescriptor(tags=FrozenOrderedSet({DiscretizationAmbientDimAxisTag()})), EinsumReductionAxis(dim=1): ReductionDescriptor(tags=FrozenOrderedSet({DiscretizationTopologicalDimAxisTag()})), EinsumReductionAxis(dim=2): ReductionDescriptor(tags=FrozenOrderedSet({DiscretizationDOFAxisTag()}))}, index_to_access_descr={'e': EinsumElementwiseAxis(dim=0), 'i': EinsumElementwiseAxis(dim=1), 'j': EinsumReductionAxis(dim=2), 'r': EinsumReductionAxis(dim=1), 'x': EinsumReductionAxis(dim=0)})}), '_in1': Einsum(axes=(Axis(tags=FrozenOrderedSet({DiscretizationElementAxisTag()})), Axis(tags=FrozenOrderedSet({DiscretizationDOFAxisTag()}))), tags={FirstAxisIsElementsTag()}, access_descriptors=((EinsumElementwiseAxis(dim=1), EinsumReductionAxis(dim=0), EinsumReductionAxis(dim=1)), (EinsumReductionAxis(dim=0), EinsumElementwiseAxis(dim=0), EinsumReductionAxis(dim=1)), (EinsumReductionAxis(dim=0), EinsumElementwiseAxis(dim=0), [......]

after:

dict_keys(['_pt_out_0_0', '_pt_out_1_0', '_pt_out_2_0'])

I agree, this is more pleasant to look at! (Thanks!)

We could probably support this on AbstractResultWithNamedArrays.

We could probably support this on AbstractResultWithNamedArrays.

I'm not sure how I could access the actual data there (ie, self._data in DictOfNamedArrays).

What I meant is add the abstract method to that interface, and add it to the other implementations.

What do you think of 914a14b?

matthiasdiener · 2024-02-19T21:22:29Z

pytato/array.py

@@ -817,7 +817,12 @@ def __iter__(self) -> Iterator[str]:
        return iter(self._data)

    def __repr__(self) -> str:
-        return "DictOfNamedArrays(" + str(self._data) + ")"
+        return "DictOfNamedArrays(" + repr(self._data) + ")"


should tags be added here too?

Probably, since it's part of the representation. Though we currently don't use "DONA" tags for anything.

added in 0278d56

matthiasdiener · 2024-02-23T20:38:01Z

pytato/loopy.py

@@ -129,6 +129,10 @@ def __len__(self) -> int:
    def __iter__(self) -> Iterator[str]:
        return iter(self._result_names)

+    # type-ignore-reason: AbstractResultWithNamedArrays returns a KeysView here
+    def keys(self) -> FrozenSet[str]:  # type: ignore[override]
+        return self._result_names


We could return dict.fromkeys(self._result_names).keys() here if you prefer.

The AbstractSet suggestion above should resolve this.

inducer · 2024-02-23T21:14:39Z

pytato/array.py

@@ -770,6 +770,10 @@ def __eq__(self, other: Any) -> bool:
        from pytato.equality import EqualityComparer
        return EqualityComparer()(self, other)

+    @abstractmethod
+    def keys(self) -> KeysView[str]:


Add to docs, too.

Done in 8ee0b82

inducer · 2024-02-23T21:20:00Z

pytato/array.py

@@ -770,6 +770,10 @@ def __eq__(self, other: Any) -> bool:
        from pytato.equality import EqualityComparer
        return EqualityComparer()(self, other)

+    @abstractmethod
+    def keys(self) -> KeysView[str]:


Suggested change

def keys(self) -> KeysView[str]:

def keys(self) -> AbstractSet[str]:

mypy doesn't really like it:

pytato/array.py:775: error: Return type "AbstractSet[str]" of "keys" incompatible with return type "KeysView[str]" in supertype "Mapping" [override]

(same for typing and collections.abc)

Ugh. Unless I'm mistaken, that seems to contradict the ABC docs?

How about Collection?

Ugh. Unless I'm mistaken, that seems to contradict the ABC docs?

I guess mypy complains whenever the return types aren't exactly the same as in the superclass?

How about Collection?

Same issue: pytato/array.py:776: error: Return type "Collection[str]" of "keys" incompatible with return type "KeysView[str]" in supertype "Mapping" [override]

inducer · 2024-02-23T21:20:18Z

pytato/loopy.py

@@ -129,6 +129,10 @@ def __len__(self) -> int:
    def __iter__(self) -> Iterator[str]:
        return iter(self._result_names)

+    # type-ignore-reason: AbstractResultWithNamedArrays returns a KeysView here
+    def keys(self) -> FrozenSet[str]:  # type: ignore[override]
+        return self._result_names


The AbstractSet suggestion above should resolve this.

matthiasdiener · 2024-11-19T21:55:52Z

This is ready for another look @inducer

* DictOfNamedArrays: better repr, keys * add tags to repr * make keys() abstract * add to doc * fix lint * remove invalid requirements.txt pytools version specification --------- Co-authored-by: Andreas Klöckner <[email protected]>

matthiasdiener force-pushed the dona-repr-keys branch from 9173fb6 to ab24c94 Compare February 19, 2024 21:18

matthiasdiener self-assigned this Feb 19, 2024

matthiasdiener commented Feb 19, 2024

View reviewed changes

matthiasdiener marked this pull request as ready for review February 19, 2024 21:21

matthiasdiener requested a review from inducer February 19, 2024 21:21

matthiasdiener commented Feb 19, 2024

View reviewed changes

matthiasdiener commented Feb 23, 2024

View reviewed changes

inducer reviewed Feb 23, 2024

View reviewed changes

matthiasdiener force-pushed the dona-repr-keys branch from 8ee0b82 to 26ed31d Compare March 22, 2024 00:11

matthiasdiener added 4 commits May 15, 2024 13:24

DictOfNamedArrays: better repr, keys

48b6d77

add tags to repr

b411c39

make keys() abstract

9aa7b82

add to doc

b5ab8d5

matthiasdiener force-pushed the dona-repr-keys branch from 26ed31d to b5ab8d5 Compare May 15, 2024 19:24

matthiasdiener requested a review from inducer May 15, 2024 19:27

matthiasdiener added 3 commits November 19, 2024 15:25

Merge branch 'main' into dona-repr-keys

8f5a319

fix lint

34fd56b

remove invalid requirements.txt pytools version specification

e7cdb1f

matthiasdiener and others added 2 commits November 19, 2024 17:44

Merge branch 'main' into dona-repr-keys

25c5311

Merge branch 'main' into dona-repr-keys

03e4901

inducer enabled auto-merge (squash) November 25, 2024 06:21

Fix sphinx doc build fail

8836d41

inducer merged commit 360ff9c into main Nov 25, 2024
11 checks passed

inducer deleted the dona-repr-keys branch November 25, 2024 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DictOfNamedArrays: better repr, keys #485

DictOfNamedArrays: better repr, keys #485

matthiasdiener commented Feb 19, 2024 •

edited

Loading

matthiasdiener Feb 19, 2024 •

edited

Loading

kaushikcfd Feb 19, 2024

inducer Feb 19, 2024

matthiasdiener Feb 19, 2024

inducer Feb 23, 2024

matthiasdiener Feb 23, 2024

matthiasdiener Feb 19, 2024

inducer Feb 19, 2024

matthiasdiener Feb 19, 2024

matthiasdiener Feb 23, 2024 •

edited

Loading

inducer Feb 23, 2024

inducer Feb 23, 2024

matthiasdiener Feb 23, 2024

inducer Feb 23, 2024

matthiasdiener Feb 23, 2024

inducer Feb 23, 2024

matthiasdiener Feb 23, 2024 •

edited

Loading

inducer Feb 23, 2024

matthiasdiener commented Nov 19, 2024

	def keys(self) -> KeysView[str]:
	def keys(self) -> AbstractSet[str]:

DictOfNamedArrays: better repr, keys #485

DictOfNamedArrays: better repr, keys #485

Conversation

matthiasdiener commented Feb 19, 2024 • edited Loading

matthiasdiener Feb 19, 2024 • edited Loading

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

matthiasdiener Feb 23, 2024 • edited Loading

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

matthiasdiener Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasdiener commented Nov 19, 2024

matthiasdiener commented Feb 19, 2024 •

edited

Loading

matthiasdiener Feb 19, 2024 •

edited

Loading

matthiasdiener Feb 23, 2024 •

edited

Loading

matthiasdiener Feb 23, 2024 •

edited

Loading