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

Fix Base.print_matrix() for big matrices #13598

Merged
merged 3 commits into from
Oct 14, 2015

Conversation

artkuo
Copy link
Contributor

@artkuo artkuo commented Oct 14, 2015

The REPL is slow displaying large vectors or matrices, e.g. eye(10^4). The reason is that Base.print_matrix() has to figure out how to display only one screenful of properly aligned columns, but in doing so, it evaluates the spacing (alignment) of all the elements, even when there are 10^10 of them. The solution is to have Base.print_matrix() evaluate a much smaller (though still conservative) subset of the matrix, so that sufficient alignment info is determined, but not overly much.

Base.print_matrix() was not originally tested, so new tests are added, including matrices that fit on the screen, are too high, too wide, or both.

This problem was discovered while developing Base.print_range() #13534, and the fix has been brought back to print_matrix.

L = alignment(X,I,1:n,c,c,ss)
r = mod((length(R)-n+1),vmod)
for i in I
else # neither rows nor cols fit, so use all 3 kinds of ellipsis
Copy link
Member

Choose a reason for hiding this comment

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

The plural of "ellipsis" is "ellipses".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but I like "ellipses" to mean the plural of those oval thingees. It hurts my head to use it to mean plural of ellipsis. Next edit I will change this to dots. But meanwhile there are bigger problems, as in despite this building successfully on my machine, it seems to fail automatic checks, for reasons beyond me. If you have any advice on how to figure this out, I'd much appreciate it.

@artkuo
Copy link
Contributor Author

artkuo commented Oct 14, 2015

No idea why this failed automatic check. I was able to make and test on my machine, and everything passed with no problems.

@stevengj
Copy link
Member

@artkuo, the failure is #11553, not your fault.

jakebolewski added a commit that referenced this pull request Oct 14, 2015
Fix Base.print_matrix() for big matrices
@jakebolewski jakebolewski merged commit 8a86270 into JuliaLang:master Oct 14, 2015
@jakebolewski
Copy link
Member

@artkuo this is a great improvement, thanks! (feel free to clean up more display code 😉)

@KristofferC
Copy link
Member

Performance increase, clearer code, better documentation, tests added... Nice job!

@artkuo artkuo deleted the quick_print_matrix branch November 12, 2015 19:48
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.

4 participants