Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Addition of Traces of MutableDenseMatrix #9072

Merged
merged 1 commit into from
Mar 22, 2015
Merged

Addition of Traces of MutableDenseMatrix #9072

merged 1 commit into from
Mar 22, 2015

Conversation

lokeshh
Copy link
Contributor

@lokeshh lokeshh commented Feb 28, 2015

Addition of Trace(X) returned TypeError when X was
MutableDenseMatrix. This was because MutableDenseMatrix
wasn't hashable. This has been fixed by introducing
hashable_content function for Trace. This function
returns sympify(X) which is hashable and hence making
MutableDenseMatrix hashable.

Fixes: #9043

@lokeshh lokeshh mentioned this pull request Feb 28, 2015
Closed
@asmeurer
Copy link
Member

asmeurer commented Mar 1, 2015

This fix is wrong. mutableDenseMatrix shouldn't be hashable, since it's mutable. The fix is to call sympify in the constrictor of Trace, which will convert a mutable matrix to an immutable one.

@lokeshh
Copy link
Contributor Author

lokeshh commented Mar 1, 2015

The fix is to call sympify in the constrictor of Trace, which will convert a mutable matrix to an immutable one.

I am not sure what that means. Does it mean that Trace object's argument needs to be converted to sympify(X) when the object is initiated?

@asmeurer
Copy link
Member

asmeurer commented Mar 1, 2015

Yes

@lokeshh lokeshh changed the title hash MutableDenseMatrix Enable addition of Traces of MutableDenseMatrix Mar 3, 2015
@lokeshh lokeshh changed the title Enable addition of Traces of MutableDenseMatrix Addition of Traces of MutableDenseMatrix Mar 3, 2015
@cbm755
Copy link
Contributor

cbm755 commented Mar 3, 2015

Yes I think so. Although I'm entirely clear on _sympify versus sympify: you might read some other functions.

This will need a test. (if this goes in first, we can always fix up the XFAIL in #9063).

@cbm755
Copy link
Contributor

cbm755 commented Mar 3, 2015

(Or if #9063 goes first, drop the XAIL here.)

@asmeurer
Copy link
Member

asmeurer commented Mar 3, 2015

You should probably do mat = sympify(mat) as the first line of __new__.

cbm755
cbm755 reviewed Mar 17, 2015
View reviewed changes
sympy/matrices/expressions/tests/test_trace.py
@@ -53,6 +53,11 @@ def test_Trace_doit():
assert Trace(q).doit(deep=False).arg == q


def test_Trace_MutableMatrix_plus():
X = Matrix([[1, 2], [3, 4]])
Copy link
Contributor

Choose a reason for hiding this comment

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

This should reference #9043, either in the test name or in a comment.

@cbm755
Copy link
Contributor

cbm755 commented Mar 17, 2015

Do I need to add XFAIL at top irregardless

XFAIL is for tests that should pass but do not. Your test does pass.

I think this should be rebase -i to squash/rewrite that first commit.

Then I'm +1.

@cbm755 cbm755 self-assigned this Mar 17, 2015
@lokeshh lokeshh force-pushed the 9043_hash_MutableDenseMatrix branch 2 times, most recently from 9b51d15 to e9158ba Compare March 19, 2015 15:31
@lokeshh
Copy link
Contributor Author

lokeshh commented Mar 19, 2015

Done! I am glad I know how to do this now.

cbm755
cbm755 reviewed Mar 19, 2015
View reviewed changes
sympy/matrices/expressions/tests/test_trace.py
@@ -53,6 +53,12 @@ def test_Trace_doit():
assert Trace(q).doit(deep=False).arg == q


def test_Trace_MutableMatrix_plus():
# FIXME: Issue #9043
Copy link
Contributor

Choose a reason for hiding this comment

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

No "FIXME" b/c you have fixed it (?)

@cbm755
Copy link
Contributor

cbm755 commented Mar 19, 2015

Put "Fixes #9043" in the commit message so it will close the bug automatically.

@lokeshh lokeshh force-pushed the 9043_hash_MutableDenseMatrix branch 2 times, most recently from 14ec843 to 5f5cd77 Compare March 19, 2015 17:50
@cbm755
Copy link
Contributor

cbm755 commented Mar 19, 2015

  1. I want the test to reference Trace(X) + Trace(X) where X MutableDenseMatrix fails "unhashable type" #9043.
  2. I do not want the test to include the words FIXME.

Please pick one:

def test_Trace_MutableMatrix_plus():
    # See issue #9043
    X = ...

or

def test_Trace_MutableMatrix_issue_9043():
    X = ...

@lokeshh
Addition of MutableDenseMatrix
750f53f 
Addition of Trace(X) returned TypeError when X was
MutableDenseMatrix. This is because MutableDenseMatrix
isn't hashable. This is fixed by calling sympify
in the constrictor of Trace, which will convert a mutable
matrix to an immutable one.

Further adding a test for this.

Fixes sympy#9043
@lokeshh lokeshh force-pushed the 9043_hash_MutableDenseMatrix branch from 5618d55 to 750f53f Compare March 20, 2015 09:57
@cbm755
Copy link
Contributor

cbm755 commented Mar 20, 2015

+1. I will merge in around 24 hours.

cbm755 added a commit that referenced this pull request Mar 22, 2015
@cbm755
Merge pull request #9072 from lokeshh/9043_hash_MutableDenseMatrix
d664cfa 
Addition of Traces of MutableDenseMatrix
@cbm755 cbm755 merged commit d664cfa into sympy:master Mar 22, 2015
@cbm755 cbm755 mentioned this pull request Mar 22, 2015
Merged
@lokeshh lokeshh deleted the 9043_hash_MutableDenseMatrix branch March 22, 2015 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cbm755 cbm755

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trace(X) + Trace(X) where X MutableDenseMatrix fails "unhashable type"
3 participants
@lokeshh @asmeurer @cbm755