-
Notifications
You must be signed in to change notification settings - Fork 525
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
mat: format complex dense matrices #1707
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1707 +/- ##
==========================================
+ Coverage 73.26% 73.29% +0.03%
==========================================
Files 510 511 +1
Lines 60132 60152 +20
==========================================
+ Hits 44056 44090 +34
+ Misses 13584 13581 -3
+ Partials 2492 2481 -11
Continue to review full report at Codecov.
|
just a drive-by comment:
ie:
thanks. PS: |
@wamuir Thank you for doing this. I've only taken a brief look, but it looks like nice work so far. I hope to be able to have a deeper look over the weekend. |
Thanks @sbinet, I wasn't aware of benchstat. Cool. Output is below. I added
Increase in clock time is statistically significant. @kortschak provided review comments, which I'll work through over the weekend and then rerun the bench. One of these comments is to use micro-benchmarking and I expect this will offer greater insight into differences. Addressing his comments may also close the current gap (e.g., his comment regarding fmt.Fprint and reflect). |
Most recent commit addresses review comments, except for the switch to sub-benchmarks. I'll need to push a second commit to update the benchmarks. I attempted some further simplification of the format functions and I think this was productive. I also added comments along the way. Updated benchstat output is below.
|
|
This PR has gone a bit stale. @wamuir are you still interested in submitting this change? |
@vladimir-ch Yes. I think this needs another review. I can make revisions. |
Please take a look.
This PR adds a
CFormat
function in gonum/mat to format complex dense matrices; this function is intended to complement the currentFormat
function. Relevant issue #641 and a relevant PR #1414, although that PR appears to have stalled. Further, PR #1414 takes the approach of wholesale duplicatingformat.go
, which may not be preferred.Summary:
Following #1414 (comment), an objective was to avoid code duplication while implementing a formatter for complex matrices. To summarize the proposed approach: value formatters are added for formatting floats and complex numbers, which are then used during matrix formatting. Since these value formatters satisfy the
fmt.Formatter
interface,fmt.State
can be passed, offering simplification in several areas (e.g., re-construction of a format string is no longer necessary). As it would be inefficient to spin up a a new formatter for each element in a matrix, a value formatter holds a buffer for reuse.To tackle widthing, a
state
type is proposed (satisfyingfmt.State
) that can be fit to a matrix and that stores information on column widths, using the existing widther type. As before, iterating through matrix elements is required. After a call toAt
, astate
will respond to subsequentWidth
calls with the relevant column width. Recommendations toward improving implementation are appreciated.Notwithstanding the need for additional internal types, this feels simpler to me on the whole and I believe this approach should make extending to new syntaxes or data types easier, if desired. Thanks for reviewing -- will address feedback/comments.
Bench:
I've included a few benchmarks below. These compare the current and proposed implementation when formatting 10x10, 100x100 and 1000x1000 dense matrices.