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.

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

Fixed #10785: Modified berkowitz_eigenvals() function #10786

Merged
merged 9 commits into from
Apr 2, 2016

Conversation

aravindkanna
Copy link
Contributor

This fixes the issue 10785.

@aravindkanna
Copy link
Contributor Author

As the empty matrix have no minors, I think it would be better to return an empty tuple for the berkowitz_minors() function.

@aravindkanna
Copy link
Contributor Author

In [87]: a.berkowitz_eigenvals()
Out[87]: {}
In [88]: a.berkowitz_minors()
Out[88]: ()

Now this would be the output for two modified functions(a is an Empty Matrix).

@aravindkanna
Copy link
Contributor Author

@moorepants , sir please review this

@aravindkanna
Copy link
Contributor Author

No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.
what is this?

@leosartaj
Copy link
Member

240 commits seem to be a lot. Can you update this with master? So only latest commits are shown.

@aravindkanna
Copy link
Contributor Author

@leosartaj , thank you 😄 can this be in?

@@ -3015,6 +3015,8 @@ def berkowitz_minors(self):

berkowitz
"""
if not self:
Copy link
Member

Choose a reason for hiding this comment

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

Is this test necessary after berkowitz has been modified to work with an empty matrix?

@aravindkanna
Copy link
Contributor Author

yeah, sorry. It is not necessary. I have created this before that. Thank you @jksuom . Can I close this now?

@jksuom
Copy link
Member

jksuom commented Apr 2, 2016

It is not necessary to totally close the PR, the tests could remain.
This is what I get with the current master for M = Matrix():

In [3]: M.berkowitz_minors()
Out[3]: (1,)
In [4]: M.berkowitz_eigenvals()
Out[4]: {}

I think you could leave these tests in this PR (with the first one properly edited). On the other hand, there seems to be no reason for special treatment of empty matrices in berkowitz_minors and berkowitz_eigenvals.
So the contents of this PR could consist of those tests alone.

@jksuom
Copy link
Member

jksuom commented Apr 2, 2016

Looks good, thanks.

@jksuom jksuom merged commit 90e8642 into sympy:master Apr 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants