-
Notifications
You must be signed in to change notification settings - Fork 107
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
Improve standard order printing #214
Conversation
Codecov Report
@@ Coverage Diff @@
## master #214 +/- ##
==========================================
- Coverage 100% 99.93% -0.07%
==========================================
Files 34 34
Lines 2755 2878 +123
==========================================
+ Hits 2755 2876 +121
- Misses 0 2 +2
Continue to review full report at Codecov.
|
Thanks for all the work you put in this PR. I would really like to merge it, but I agree that the order of printing should be fixed first. Unfortunately, I also couldn't come up with a good solution for this new issue when going through your code. Basically, in the printing loop in the I have one more unrelated question: could the indexing with |
src/printing.jl
Outdated
half_screen_rows = typemax(Int) | ||
end | ||
pad = ndigits(max(S.m,S.n)) | ||
k = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assignment of k
here is overwritten later when k
is used as loop variable, so this line here is not needed.
src/printing.jl
Outdated
elseif k == half_screen_rows | ||
print(io, sep, '\u22ee') | ||
end | ||
k += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again this line has no effect since k
is overwritten by the next step in the loop.
I will reconstruct
No. For printing case, only part of the matrix entries are printed in REPL. So we don't need all matrix entries information. We need several thousand entries information at most. In that case even if a matrix size becomes larger and lager, computational cost is almost constant (since the number of printed entries is fixed). However |
I fix |
Thanks for the update. Everything looks to be fine now. I am just a bit worried because there is entire functions that are never called during testing (e.g. |
Sorry. Those functions are not necessary. |
I have improved standard order printing by removing permutation.
Present issues
As far as I know, there are two issues related to standard order printing.
Improved version
The above problems are caused by permutation. So I remove it and have modified pretty-print for
Vector
andMatrix
in order to support standard order print.New issue
By this modification, I fix above problems (error and printing speed).
However as you look above output, about sparse matrix it is not same as permutation version (current version).
As you see following small example, the printed order is not same although information is identical.
I want to print it same order, but I don't have the good idea.
Is there a good solution?
My environment