Added Documentation for SDM class #21163

ks147 · 2021-03-25T07:32:27Z

References to other Issues or PRs

Brief description of what is fixed or changed

Added Documentation for SDM class

Other comments

Release Notes

polys
- Added Documentation for SDM class

sympy-bot · 2021-03-25T07:32:38Z

✅

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

polys
- Added Documentation for SDM class (#21163 by @ks147)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.9.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs

#### Brief description of what is fixed or changed
Added Documentation for SDM class

#### Other comments


#### Release Notes

<!-- BEGIN RELEASE NOTES -->
* polys
  * Added Documentation for SDM class
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

oscarbenjamin · 2021-03-25T10:04:06Z

sympy/polys/matrices/sdm.py

+        >>> elemsdict = {0:{1:QQ(2)}, 1:{}}
+        >>> A = SDM(elemsdict, (2, 2), QQ)
+        >>> A
+        {0: {1: 2}, 1: {}}


The elemsdict should not have any empty dicts in there. This docstring should explain that it is the responsibility of whoever creates elemsdict to remove zero rows and zero elements.

Also this should show how to construct a DomainMatrix from the SDM.

I think it would be better to move this docstring to be the main class docstring and also to show examples of using the SDM e.g. for adding and multiplying

I'll do that

oscarbenjamin · 2021-03-25T10:05:41Z

sympy/polys/matrices/sdm.py

+
+        >>> from sympy.polys.matrices.sdm import SDM
+        >>> from sympy import QQ
+        >>> ddm = [[QQ(1, 2), 0], [0, QQ(3, 4)]]


The 0 should be QQ(0).

I'll change it everywhere

oscarbenjamin · 2021-03-25T10:24:39Z

sympy/polys/matrices/sdm.py

+
+        >>> from sympy import QQ
+        >>> from sympy.polys.matrices.sdm import SDM
+        >>> A = SDM({0:{0:QQ(1), 1:QQ(2)}, 1:{0: QQ(3), 1: QQ(4)}}, (2, 2), QQ)


The spacing around colons is inconsistent.

oscarbenjamin · 2021-03-25T10:25:42Z

sympy/polys/matrices/sdm.py

+        >>> from sympy.polys.matrices.sdm import SDM
+        >>> A = SDM({0:{0:QQ(1), 1:QQ(2)}, 1:{0: QQ(3), 1: QQ(4)}}, (2, 2), QQ)
+        >>> A.charpoly()
+        [1, -5, -2]


It would be good to show here how to construct a Poly from these e.g.:

x = Symbol('x') p = Poly(A.charpoly(), x, domain=A.domain) p

oscarbenjamin · 2021-03-25T10:28:02Z

sympy/polys/matrices/sdm.py

+        >>> A = SDM({0:{1: ZZ(2)}, 1:{0:ZZ(1)}}, (2, 2), ZZ)
+        >>> B = SDM({0:{0:ZZ(2), 1:ZZ(3)}, 1:{0:ZZ(4)}}, (2, 2), ZZ)
+        >>> A.matmul(B)
+        {0: {0: 8}, 1: {0: 2, 1: 3}}


For each of these methods it would be good to point out that they are also used by the operators so e.g. this could show that you get the same from A*B.

These docstrings should also explain when the domains are are required to match or what constraints there are on the shapes of the matrices for the operation to be defined.

You actually don't get the same from A * B.

A * B
Traceback (most recent call last):
File "", line 1, in
TypeError: unsupported operand type(s) for *: 'SDM' and 'SDM'

This is what you get with A*B

I think __add__(A, B) needs to be implemented for SDM class

Oh yeah, I think it's A @ B or __matmul__. Is that not implemented for SDM?

Perhaps it is better to use * anyway. Either way SDM and DDM should be made consistent so can you compare how DDM works?

Consistent in what ways? You want both to have the same functionality right, is that what you're saying

By the way * does not work in DDM either

A = DDM([[ZZ(1), ZZ(2)], [ZZ(2), ZZ(3)]], (2, 2), ZZ) B = DDM([[ZZ(3), ZZ(4)], [ZZ(1), ZZ(2)]], (2, 2), ZZ) A * B Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: unsupported operand type(s) for *: 'DDM' and 'DDM

I'll have to make __mul__ in DDM consistent with definition in DomainMatrix

I mean that when using methods like A * B or A.matmul(B) etc then all behaviour should be exactly the same whether A and B are both DDM or both SDM. These two classes are supposed to be completely interchangeable in that sense. So if you are changing any part of the behaviour of SDM you should check that DDM behaves in exactly the same way.

I'll have to make __mul__ in DDM consistent with definition in DomainMatrix

Note that DDM and DomainMatrix do not need to be consistent as they are not intended to be interchangeable. It is DDM and SDM that need to be consistent. For now I think that SDM should just use @ for matrix multiplication just like DDM does. We can consider changing that in future but these two classes need to remain equivalent.

…,etc.

oscarbenjamin · 2021-04-19T21:48:23Z

sympy/polys/matrices/sdm.py

+    >>> elemsdict = {0:{1:QQ(2)}}
+    >>> A = SDM(elemsdict, (2, 2), QQ)
+    >>> A
+    {0: {1: 2}}


The most useful thing to show here would be what this looks like as a Matrix. I don't think that the meaning of the dict of dicts structure is obvious without being explained.

oscarbenjamin · 2021-04-19T21:49:39Z

sympy/polys/matrices/sdm.py

+    We can manipulate :py:class:`~.SDM` the same way
+    as a Matrix class
+
+    >>> from sympy import ZZ
+    >>> A = SDM({0:{1: ZZ(2)}, 1:{0:ZZ(1)}}, (2, 2), ZZ)
+    >>> B  = SDM({0:{0: ZZ(3)}, 1:{1:ZZ(4)}}, (2, 2), ZZ)
+    >>> A + B
+    {0: {0: 3, 1: 2}, 1: {0: 1, 1: 4}}
+
+    Multiplication
+
+    >>> A = SDM({0:{1: ZZ(2)}, 1:{0:ZZ(1)}}, (2, 2), ZZ)
+    >>> B = SDM({0:{0: ZZ(3)}, 1:{1:ZZ(4)}}, (2, 2), ZZ)
+    >>> A*B
+    {0: {1: 8}, 1: {0: 3}}
+    >>> A*ZZ(2)
+    {0: {1: 4}, 1: {0: 2}}


These examples can be condensed. We don't need to create new matrices for each demonstration.

sympy/polys/matrices/sdm.py

oscarbenjamin · 2021-04-19T21:51:30Z

sympy/polys/matrices/sdm.py

+
+        >>> from sympy.polys.matrices.sdm import SDM
+        >>> from sympy import QQ
+        >>> elemsdict = {0:{1:2}, 1:{}}


The empty dict shouldn't be there. The 2 should be QQ(2)

oscarbenjamin · 2021-04-19T21:53:27Z

sympy/polys/matrices/sdm.py

+        >>> from sympy import QQ
+        >>> A = SDM.zeros((2, 3), QQ)
+        >>> A
+        {}


This would be easier to understand if you also show what the corresponding matrix would be.

You could show what A.to_Matrix() gives here

sympy/polys/matrices/sdm.py

oscarbenjamin · 2021-04-19T21:57:10Z

sympy/polys/matrices/sdm.py

@@ -308,7 +751,7 @@ def sdm_irref(A):
    >>> pivots
    [0, 1]

-    The analogous calculation with :py:class:`~.Matrix` would be
+    The analogous calculation with Matrix class would be


Why is this changed?

ks147 · 2021-05-01T08:41:35Z

@oscarbenjamin please review

oscarbenjamin · 2021-05-01T09:02:33Z

sympy/polys/matrices/sdm.py

+    >>> elemsdict = {0:{1:QQ(1, 2)}}
+    >>> A = SDM(elemsdict, (2, 2), QQ)
+    >>> A
+    {0: {1: 1/2}}


It would be good so short the result of A.to_Matrix() here.

I don't think SDM class has to_Matrix(), DomainMatrix on the other hand does have that functionality

oscarbenjamin · 2021-05-01T09:05:10Z

sympy/polys/matrices/sdm.py

        MT = sdm_transpose(M)
-        return M.new(MT, M.shape[::-1], M.domain)
+        return MT


Why is this changed? Does this return a dict or SDM?

sympy/polys/matrices/sdm.py

oscarbenjamin · 2021-05-01T18:50:01Z

Okay, looks good.

ks147 added 3 commits March 22, 2021 10:42

Adding Docs for SDM class

a74fee9

Fixed some of the examples and their output

5d11c15

Fixed output of some examples, now passing doctest

10577b0

ks147 changed the title ~~Sdm docs~~ Added Documentation for SDM class Mar 25, 2021

oscarbenjamin reviewed Mar 25, 2021

View reviewed changes

ks147 and others added 5 commits March 27, 2021 15:22

Made changes suggested in code review and added funcs __add__,__mul__…

e8314b6

…,etc.

Moved docstring of __init__ to main class function

53925c6

Made __mul__ in SDM consistent with DomainMatrix

3db6c8b

Removed reference to Matrix class to pass sphinx test

e749686

Merge branch 'master' into SDM_Docs

eb45084

oscarbenjamin reviewed Apr 19, 2021

View reviewed changes

sympy/polys/matrices/sdm.py Show resolved Hide resolved

oscarbenjamin reviewed Apr 19, 2021

View reviewed changes

sympy/polys/matrices/sdm.py Show resolved Hide resolved

oscarbenjamin reviewed Apr 19, 2021

View reviewed changes

Made changes suggested in code review

35e636c

oscarbenjamin reviewed May 1, 2021

View reviewed changes

sympy/polys/matrices/sdm.py Outdated Show resolved Hide resolved

ks147 added 2 commits May 1, 2021 16:36

Merge branch 'master' of https://github.com/sympy/sympy into SDM_Docs

f26550a

Made changes suggested in code review

d97e331

oscarbenjamin merged commit bf1f842 into sympy:master May 1, 2021

Uh oh!

Added Documentation for SDM class #21163

Added Documentation for SDM class #21163

Uh oh!

Conversation

ks147 commented Mar 25, 2021

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

Uh oh!

sympy-bot commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

ks147 Mar 27, 2021 • 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ks147 commented May 1, 2021

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!

Uh oh!

oscarbenjamin commented May 1, 2021

Uh oh!

Uh oh!

sympy-bot commented Mar 25, 2021 •

edited

Loading

ks147 Mar 27, 2021 •

edited

Loading