Closed giangiac closed 4 months ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
I will review that in the afternoon, thanks for the contribution, seems nice!
Revert some of the suggested changes:
str
object is immutable and do not support item assignment
A few general comments.
The current code has not been formatted by an automatic tool. I can do that for you, or you can do it, your call. You can use
black
orruff
. (I want to add that to CI in the short term).A few parameters / hard-coded values could be exposed as parameters. This is for example the case of:
h_space
that can be a parameter with a default value of5
,cross_mark
to let users replace that with a non-unicode character if they know that their terminal / text-display-thing does not support unicode.- The number
2
that seems to bev_space
(vertical space) and that appear 3 times in the code (I think).Also, it might be worth implementing the "bounding-box coordinate shifting" algorithm to avoid negative coordinates directly as a (private) method in the
ComposedTemplate
class, and re-use that method both inComposedTemplate
and in your implementation here. That would also reduce the length and complexity of thedisplay_template_ascii
function.Seeing the operations you are performing (replacing rows/columns), it might be worth using a
numpy.ndarray
withdtype
ofnumpy.str_
. This would likely improve readability, performance and memory consumption.
Thanks for the quick review!
It would be great if you could apply the automatic formatting tool. Thanks!
I think that using a
numpy.ndarray
ofdtype = numpy.str_
would still be worth. I can try to hack a version with that if you do not want to test yourself, to see what it ends up looking like.
I tried briefly, and it seems to shift code complexity from string manipulation to index manipulation. I personally prefer index manipulation, but string manipulation is way more readable, so please ignore that part of my comment. Trying numpy
-based implementation might not be worth.
I think that using a
numpy.ndarray
ofdtype = numpy.str_
would still be worth. I can try to hack a version with that if you do not want to test yourself, to see what it ends up looking like.I tried briefly, and it seems to shift code complexity from string manipulation to index manipulation. I personally prefer index manipulation, but string manipulation is way more readable, so please ignore that part of my comment. Trying
numpy
-based implementation might not be worth.
I'd also value readability more than efficiency. We are printing to screen, a functionality that is not meaningful for ASCII with more than a few 100s lines.
A few more notes and ask for changes.
Also see giangiac#2 for formatting.
Thanks! Merged.
Method added in
<repo>/src/tqec/templates/display.py
.It is tested in notebook
<repo>/notebooks/doubling_distance.ipynb
.Example of output: