Format floating-point values in the Rust circuit drawer#15899
Format floating-point values in the Rust circuit drawer#15899eliarbel merged 4 commits intoQiskit:mainfrom
Conversation
|
One or more of the following people are relevant to this code:
|
Cryoris
left a comment
There was a problem hiding this comment.
Thanks, this output is much more legible! I left some comments below.
| } | ||
|
|
||
| /// Formats the input number based on the formatting options. | ||
| /// This Can be called multiple times, but the internal buffer is overwritten on each call. |
There was a problem hiding this comment.
Are we testing anywhere that this can be called multiple times? Looking at the code it seems that a new formatter is instantiated every time when we call .format.
There was a problem hiding this comment.
While I did call one formatter multiple times while I was testing this manually, I thought that since the crate is built around the notion of using a static buffer anyway, it's not worth to add explicit tests for that and that we can just go with the other circuit output tests which for the formatting aspect itself.
That said, I've added such test anyway in d9c2307 since a) it woudln't hurt and b) I want to extend it after #15917 is merged to have a test which covers both
| .iter() | ||
| .map(|param| match param { | ||
| Param::Float(f) => f.to_string(), | ||
| Param::Float(f) => F64UiFormatter::new(5).format(*f).to_string(), |
There was a problem hiding this comment.
What do you think of making the formatter an attribute of the TextDrawer? That would allow us to easily expose this as config later on, and allows to only allocate the buffer once and reuse it (ok likely an unnecessary optimization, but still haha)
There was a problem hiding this comment.
I thought about it while implementing this and decided to skip it for now. Most of the functions in TextDrawer are associative but should be instance methods IMO to make the code more idiomatic, but I would defer this to a different PR. I still wrote the formatter as a struct instead of a simple function though to allow easy reuse of the buffer in case this will ever become a bottleneck, but for now I went with the more straightforward usage of instantiating and calling it on-the-fly.
Regarding performance, TBH, there are a lot of performance and memory optimization opportunities in the Rust circuit writer, but I think for now, besides functionality, the major consideration with the writer should be readability and easy extendibility.
There was a problem hiding this comment.
Ok sounds good to me. Since we also are testing now that the re-usable buffer works, I'm good deferring this 👍🏻
This commit adds formatting functionality to render floating-point values (e.g. rotation angles) in a way similar to the `g` formatting flag in Python.
* change input parameter of the formatter to usize * add tests
d9c2307 to
fd79443
Compare
Cryoris
left a comment
There was a problem hiding this comment.
Just one question, otherwise LGTM! It might be nice to expose the number of significant bits as option at some point in the future, but I'm favor of the short 5-digits for drawing as default.
I'm thinking it might be nice to split the code into several files which should reside in their own sub-directory. Especially with #16063 which is going to add a lot of code relatively. Once this is done, we can put the formatter in its own file (or in a general utils file) and generalize it more, including having that parameter accept |
This commit adds formatting functionality to render floating-point values (e.g. rotation angles) in a way similar to the
gformatting flag in Python.This PR addresses item 3 in #15849
Details and comments
For example, this results in an output like this:
instead of: