Canonicalization of Hadamard Product #16685

sylee957 · 2019-04-19T10:19:37Z

References to other Issues or PRs

Brief description of what is fixed or changed

With matrix of ones implemented in #16676, we may use the identity to make hadamard product more canonical.

Associative (Default)
Commutative
Identity for Hadamard product by one matrix
Absorbing to zero matrix when multiplied with zero matrix
Rewriting to Hadamard power when repetitive matrices are multiplied.
(Optional) Reduce to diagonal matrix when multiplied with IdentityMatrix (Or any rectangular eye) After the decision is made for Rename DiagonalizeVector? #16682

Other comments

This is still work in progress, and I will add some tests if the design is set

I also think that HadamardProduct can automatically unpack its argument, if given a single argument, like in SymPy's Add, Mul.
As it is confusing that
HadamardProduct(A) == A is giving False because it does not unpacks single arguent
but hadamard_product(A) == A is giving True.
And the printing is not suggestive that HadamardProduct is wrapped on this.
Though changing this would make some tests fail, and there may be some discussion for design change?

Release Notes

matrices
- Improved canonicalization for HadamardProduct
- Deleted rules from matrices.expressions.hadamard

sympy-bot · 2019-04-19T10:19:47Z

✅

Hi, I am the SymPy bot (v147). 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:

matrices
- Improved canonicalization for HadamardProduct (#16685 by @sylee957 and @Upabjojr)
- Deleted rules from matrices.expressions.hadamard (#16685 by @sylee957 and @Upabjojr)

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

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

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
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed

With matrix of ones implemented in #16676, we may use the identity to make hadamard product more canonical.

- [X] Associative (Default)
- [X] Commutative
- [X] Identity for Hadamard product by one matrix
- [X] Absorbing to zero matrix when multiplied with zero matrix
- [x] Rewriting to Hadamard power when repetitive matrices are multiplied.
- [ ] (Optional) Reduce to diagonal matrix when multiplied with `IdentityMatrix` (Or any rectangular `eye`) After the decision is made for #16682 

#### Other comments

This is still work in progress, and I will add some tests if the design is set

I also think that `HadamardProduct` can automatically unpack its argument, if given a single argument, like in SymPy's `Add`, `Mul`.
As it is confusing that
`HadamardProduct(A) == A` is giving `False` because it does not unpacks single arguent
but `hadamard_product(A) == A` is giving `True`.
And the printing is not suggestive that `HadamardProduct` is wrapped on this.
Though changing this would make some tests fail, and there may be some discussion for design change?

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
- matrices
  - Improved canonicalization for `HadamardProduct`
  - Deleted `rules` from `matrices.expressions.hadamard`
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

codecov · 2019-04-20T17:26:49Z

Codecov Report

Merging #16685 into master will decrease coverage by 0.019%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##            master    #16685      +/-   ##
============================================
- Coverage   73.847%   73.827%   -0.02%     
============================================
  Files          619       619              
  Lines       159329    159406      +77     
  Branches     37388     37414      +26     
============================================
+ Hits        117660    117685      +25     
- Misses       36241     36263      +22     
- Partials      5428      5458      +30

Upabjojr · 2019-04-20T17:29:53Z

I've added some tests and made some minor fix.

It looks good, I need this PR to go on with the matrix derivatives algorithm.

…s the product order to depend on the Python hashing values.

sympy/matrices/expressions/hadamard.py

…57/sympy into hadamard_canonicalization

Upabjojr · 2019-04-22T12:08:58Z

I would put the examples in the HadamardProduct documentation. The function canonicalize is meant to be private.

Upabjojr · 2019-04-22T12:10:53Z

For diagonal matrix checks:

from sympy.assumptions import ask, Q
ask(Q.diagonal(mat))  # <== this returns True if the matrix is diagonal

Though I would use it with care, as slowdowns might be expected.

oscarbenjamin · 2019-04-22T18:56:42Z

sympy/matrices/expressions/hadamard.py

+    =====
+
+    As Hadamard product is associative, all the nested products can be
+    flattened to the level 1.


As the Hadamard product is associative nested products can be flattened.

oscarbenjamin · 2019-04-22T18:57:28Z

sympy/matrices/expressions/hadamard.py

+    flattened to the level 1.
+
+    As Hadamard product is commutative, all the arguments can be sorted to
+    the canonical form.


The Hadamard product is commutative so factors can be sorted for canonical form.

oscarbenjamin · 2019-04-22T18:57:47Z

sympy/matrices/expressions/hadamard.py

+    the canonical form.
+
+    Matrix of only ones is an identity for Hadamard product,
+    so every ``OneMatrix`` can be removed.


A matrix of only ones...

oscarbenjamin · 2019-04-22T18:58:31Z

sympy/matrices/expressions/hadamard.py

+    Matrix of only ones is an identity for Hadamard product,
+    so every ``OneMatrix`` can be removed.
+
+    Matrix of only zeros will make Hadamard product zero.


Any zero matrix will make the whole product a zero matrix.

sympy/matrices/expressions/hadamard.py

oscarbenjamin · 2019-04-22T18:59:52Z

sympy/matrices/expressions/hadamard.py

+
+    # Rewriting with HadamardPower
+    if isinstance(x, HadamardProduct):
+        tally = dict()


Could use collections.Counter

or multiset from iterables:

>>> multiset('mississippi') {'i': 4, 'm': 1, 'p': 2, 's': 4}

so

tally = multiset(x.args)

I've done some experiment in cpython 3.6

from sympy.utilities.iterables import multiset from collections import Counter l = 'abc'+'c'*999999

%timeit Counter(l) 349 ms ± 53.7 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
%timeit multiset(l) 578 ms ± 36.4 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

I guess the one from the official library can use C function to speed up
https://github.com/python/cpython/blob/f2f55e7f03d332fd43bc665a86d585a79c3b3ed4/Modules/_collectionsmodule.c#L2237

smichr · 2019-04-27T12:53:45Z

sympy/matrices/expressions/hadamard.py

+        x = HadamardProduct(*new_arg)
+
+    # Commutativity
+    fun = condition(


what aspect of this relates to commutativity? After using the tally, things are going to be reordered, aren't they?

I think the fact that things are getting reordered will make a reason to sort lastly.

smichr · 2019-05-10T05:17:26Z

Is there more you wanted to do with this?

sylee957 · 2019-05-10T05:29:05Z

Things like boilerplating can better be cleaned up later after some big progress in the module is made, regarding the comments.

I had some other things to do, but I think that there is no problem for merging this now.

asmeurer · 2019-05-10T19:52:59Z

Should it use AssocOp? I don't know if there are issues using it in the matrix expressions.

asmeurer · 2019-05-10T19:54:24Z

I guess MatAdd and MatMul use it via subclassing from Add and Mul. Although I added an XXX comment that that might not be desirable. So I'm really not sure if using AssocOp for this would be a good idea or not.

sylee957 · 2019-05-11T04:45:47Z

I considered AssocOp, but I think that there is a problem for defining identity because the matrix operations can only be defined after the dimensions are known.

sylee957 added 2 commits April 19, 2019 17:35

Canonicalization for hadamard product

639e28a

Add comments and docstrings

206acc2

sylee957 changed the title ~~Canonicalization of Hadamard Product~~ [WIP] Canonicalization of Hadamard Product Apr 19, 2019

Add onematrix to import

babdfcb

sylee957 added the matrices.expressions label Apr 20, 2019

sylee957 and others added 7 commits April 20, 2019 17:46

Should not exhaust if flattened

c1ca855

Change comment

1fbfe1d

Remove the unpacking from constructor

dd44b53

Add rewriting to hadamard power

61e1e8c

Delete obsolete doc

4ca38de

merge master

e34f9c3

minor fixes and more tests for Hadamard product canonicalization

a718dc2

add default_sort_key to Hadamard product canonicalization. This avoid…

30867e3

…s the product order to depend on the Python hashing values.

oscarbenjamin reviewed Apr 20, 2019

View reviewed changes

sympy/matrices/expressions/hadamard.py Outdated Show resolved Hide resolved

oscarbenjamin reviewed Apr 20, 2019

View reviewed changes

sympy/matrices/expressions/hadamard.py Show resolved Hide resolved

oscarbenjamin reviewed Apr 20, 2019

View reviewed changes

sympy/matrices/expressions/hadamard.py Outdated Show resolved Hide resolved

oscarbenjamin reviewed Apr 20, 2019

View reviewed changes

sympy/matrices/expressions/hadamard.py Outdated Show resolved Hide resolved

sylee957 added 8 commits April 21, 2019 14:08

Improve docs

52aa1c5

Comment todo

c64bc8d

Add rewriting as hadamard power

fc75c34

Merge branch 'hadamard_canonicalization' of https://github.com/sylee9…

377c430

…57/sympy into hadamard_canonicalization

Sort last

7a5db2a

Unpack the list

b106513

Use different constructor

9c446e7

Improve examples

d18a159

Merge branch 'master' into hadamard_canonicalization

c458f22

oscarbenjamin reviewed Apr 22, 2019

View reviewed changes

sympy/matrices/expressions/hadamard.py Outdated Show resolved Hide resolved

oscarbenjamin reviewed Apr 22, 2019

View reviewed changes

smichr reviewed Apr 27, 2019

View reviewed changes

sylee957 added 2 commits April 30, 2019 15:17

Refine docs

be59130

Use collections.Counter

cd45432

sylee957 changed the title ~~[WIP] Canonicalization of Hadamard Product~~ Canonicalization of Hadamard Product May 10, 2019

smichr merged commit 0b09027 into sympy:master May 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Canonicalization of Hadamard Product #16685

Canonicalization of Hadamard Product #16685

sylee957 commented Apr 19, 2019 •

edited

sympy-bot commented Apr 19, 2019 •

edited

codecov bot commented Apr 20, 2019 •

edited

Upabjojr commented Apr 20, 2019

Upabjojr commented Apr 22, 2019

Upabjojr commented Apr 22, 2019

oscarbenjamin Apr 22, 2019

oscarbenjamin Apr 22, 2019

oscarbenjamin Apr 22, 2019

oscarbenjamin Apr 22, 2019

oscarbenjamin Apr 22, 2019

smichr Apr 27, 2019

sylee957 Apr 30, 2019

smichr Apr 27, 2019

sylee957 May 10, 2019

smichr commented May 10, 2019

sylee957 commented May 10, 2019

asmeurer commented May 10, 2019

asmeurer commented May 10, 2019

sylee957 commented May 11, 2019

Canonicalization of Hadamard Product #16685

Canonicalization of Hadamard Product #16685

Conversation

sylee957 commented Apr 19, 2019 • edited

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Apr 19, 2019 • edited

codecov bot commented Apr 20, 2019 • edited

Codecov Report

Upabjojr commented Apr 20, 2019

Upabjojr commented Apr 22, 2019

Upabjojr commented Apr 22, 2019

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

smichr commented May 10, 2019

sylee957 commented May 10, 2019

asmeurer commented May 10, 2019

asmeurer commented May 10, 2019

sylee957 commented May 11, 2019

sylee957 commented Apr 19, 2019 •

edited

sympy-bot commented Apr 19, 2019 •

edited

codecov bot commented Apr 20, 2019 •

edited