Fix QROM documentation latex rendering issues#1859
Open
Gopal-Dahale wants to merge 2 commits into
Open
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request replaces \text{} with \mathrm{} in LaTeX expressions across several QROM-related notebooks and the base class docstrings. Feedback indicates that underscores within these new \mathrm{} blocks must remain escaped with a backslash to prevent them from being interpreted as subscript triggers in LaTeX. Furthermore, several LaTeX expressions in the cost sections were identified as malformed due to incorrect command syntax and mismatched parentheses. The reviewer also emphasized that the source Python files should be updated directly to ensure that documentation remains consistent after regeneration.
Comment on lines
+91
to
+94
| \mathrm{data[i].shape} = \mathrm{selection_shape} + \mathrm{target_shape[i]} | ||
| $$ | ||
|
|
||
| Note that the $\text{selection\_shape}$ should be same across all classical datasets to be | ||
| Note that the $\mathrm{selection_shape}$ should be same across all classical datasets to be |
Contributor
There was a problem hiding this comment.
The replacement of \text{} with \mathrm{} for variable names containing underscores requires preserving the backslash escape for the underscore (i.e., \_). Without the escape, LaTeX interprets the underscore as a subscript trigger, leading to incorrect rendering (e.g., \mathrm{selection_shape} renders as $selection_{shape}$ instead of $selection_shape$).
Additionally, these changes are inconsistent with the manual updates in the Jupyter notebooks (e.g., qroam_clean.ipynb), where the underscores remain escaped. Since those notebook sections are autogenerated from this docstring, the source here must be correct to avoid regressions when the documentation is next rebuilt.
Suggested change
| \mathrm{data[i].shape} = \mathrm{selection_shape} + \mathrm{target_shape[i]} | |
| $$ | |
| Note that the $\text{selection\_shape}$ should be same across all classical datasets to be | |
| Note that the $\mathrm{selection_shape}$ should be same across all classical datasets to be | |
| \mathrm{data[i].shape} = \mathrm{selection\_shape} + \mathrm{target\_shape[i]} | |
| $$ | |
| Note that the \mathrm{selection\_shape} should be same across all classical datasets to be |
Comment on lines
+104
to
+127
| to be loaded must have the same $\mathrm{selection_shape}$, which is a tuple of iteration | ||
| lengths over each dimension of the dataset (i.e. the range for each nested for-loop). | ||
|
|
||
| In order to load a data set with $\text{selection\_shape} == (P, Q, R, S)$ the QROM bloq | ||
| In order to load a data set with $\mathrm{selection_shape} == (P, Q, R, S)$ the QROM bloq | ||
| needs four selection registers with bitsizes $(p, q, r, s)$ where each of | ||
| $p,q,r,s \geq \log_2{P}, \log_2{Q}, \log_2{R}, \log_2{S}$. | ||
|
|
||
| In general, to load $K$ dimensional data, we use $K$ named selection registers | ||
| $(\text{selection}_0, \text{selection}_1, ..., \text{selection}_k)$ to index and | ||
| $(\mathrm{selection}_0, \mathrm{selection}_1, ..., \mathrm{selection}_k)$ to index and | ||
| load the data. For the $i$'th selection register, its size is configured using | ||
| attribute $\text{selection\_bitsizes[i]}$ and the iteration range is configued | ||
| using $\text{data\_or\_shape[0].shape[i]}$. | ||
| attribute $\mathrm{selection_bitsizes[i]}$ and the iteration range is configued | ||
| using $\mathrm{data_or_shape[0].shape[i]}$. | ||
|
|
||
| ### Number of target registers, their quantum datatype and shape | ||
| QROM bloq uses one target register for each entry corresponding to classical dataset in the | ||
| tuple `data_or_shape`. Thus, to load $L$ classical datsets, we use $L$ names target registers | ||
| $(\text{target}_0, \text{target}_1, ..., \text{target}_L)$ | ||
| $(\mathrm{target}_0, \mathrm{target}_1, ..., \mathrm{target}_L)$ | ||
|
|
||
| Each named target register has a bitsize $b_{i}=\text{target\_bitsizes[i]}$ that represents | ||
| Each named target register has a bitsize $b_{i}=\mathrm{target_bitsizes[i]}$ that represents | ||
| the size of the register and depends upon the maximum value of individual elements in the | ||
| $i$'th classical dataset. | ||
|
|
||
| Each named target register has a shape that can be configured using attribute | ||
| $\text{target\_shape[i]}$ that represents the number of target registers if the output to load | ||
| $\mathrm{target_shape[i]}$ that represents the number of target registers if the output to load |
Contributor
There was a problem hiding this comment.
Similar to the previous comment, underscores inside \mathrm{} must be escaped with a backslash to render correctly as literal underscores. This applies to several instances in this section (selection_shape, selection_bitsizes, data_or_shape, target_bitsizes, target_shape). I have also included fixes for a few typos found in these lines ("describe", "configued", "datsets", "names").
to be loaded must have the same \mathrm{selection\_shape}, which is a tuple of iteration
lengths over each dimension of the dataset (i.e. the range for each nested for-loop).
In order to load a data set with \mathrm{selection\_shape} == (P, Q, R, S) the QROM bloq
needs four selection registers with bitsizes (p, q, r, s) where each of
p,q,r,s \geq \log_2{P}, \log_2{Q}, \log_2{R}, \log_2{S}.
In general, to load K dimensional data, we use K named selection registers
(\mathrm{selection}_0, \mathrm{selection}_1, ..., \mathrm{selection}_k) to index and
load the data. For the i'th selection register, its size is configured using
attribute \mathrm{selection\_bitsizes[i]} and the iteration range is configured
using \mathrm{data\_or\_shape[0].shape[i]}.
### Number of target registers, their quantum datatype and shape
QROM bloq uses one target register for each entry corresponding to classical dataset in the
tuple `data_or_shape`. Thus, to load L classical datasets, we use L named target registers
(\mathrm{target}_0, \mathrm{target}_1, ..., \mathrm{target}_L)
Each named target register has a bitsize b_{i}=\mathrm{target\_bitsizes[i]} that represents
the size of the register and depends upon the maximum value of individual elements in the
i'th classical dataset.
Each named target register has a shape that can be configured using attribute
\mathrm{target\_shape[i]} that represents the number of target registers if the output to load| corresponding to the size of each dimension of the selection_shape | ||
| $(S_1, S_2, ..., S_K)$. Should be the same length as the selection shape of | ||
| each of the datasets and $2**\text{selection\_bitsizes[i]} >= S_i$ | ||
| each of the datasets and $2**\mathrm{selection_bitsizes[i]} >= S_i$ |
Contributor
There was a problem hiding this comment.
The underscore in selection_bitsizes must be escaped.
Suggested change
| each of the datasets and $2**\mathrm{selection_bitsizes[i]} >= S_i$ | |
| each of the datasets and $2**\mathrm{selection\_bitsizes[i]} >= S_i$ |
Comment on lines
+158
to
+166
| "all dimensions (i.e. $\\mathcal{O}(\\mathrm{np.prod(\\mathrm{selection\\_shape})}$).\n", | ||
| "\n", | ||
| "### Clifford Cost\n", | ||
| "To load a classical dataset into a target register of bitsize $b$ and shape\n", | ||
| "$\\text{target\\_shape}$, the clifford cost of this QROM scales as\n", | ||
| "$\\mathcal{O}(b \\cdot \\text{np.prod(selection\\_shape+target\\_shape)})\n", | ||
| "=\\mathcal{O}(b \\cdot \\text{np.prod(data.shape)})$. This is because we need $\\mathcal{O}(b)$\n", | ||
| "$\\mathrm{target\\_shape}$, the clifford cost of this QROM scales as\n", | ||
| "$\\mathcal{O}(b \\cdot \\mathrm{np.prod(selection\\_shape+target\\_shape)})\n", | ||
| "=\\mathcal{O}(b \\cdot \\mathrm{np.prod(data.shape)})$. This is because we need $\\mathcal{O}(b)$\n", | ||
| "CNOT gates to load 1 classical data element in the target register and for each of the\n", | ||
| "$\\text{np.prod(selection\\_shape)}$ selection indices, we have $\\text{np.prod(target\\_shape)}$\n", | ||
| "$\\mathrm{np.prod(selection\\_shape)}$ selection indices, we have $\\mathrm{np.prod(target\\_shape)}$\n", |
Contributor
There was a problem hiding this comment.
The LaTeX expressions in the cost section are malformed. Specifically:
\mathrmis a command that expects its argument in curly braces{...}, not parentheses(...). Using\mathrm(...)only applies the font to the opening parenthesis.- There are mismatched parentheses in the
\mathcal{O}(...)expressions (e.g., line 158 and 163 are missing closing parentheses). - The source file
qrom.py, from which this section is autogenerated, has not been updated in this PR. Manual changes here will be lost upon the next documentation build. Please updateqrom.pyand regenerate the notebooks.
Suggested change
| "all dimensions (i.e. $\\mathcal{O}(\\mathrm{np.prod(\\mathrm{selection\\_shape})}$).\n", | |
| "\n", | |
| "### Clifford Cost\n", | |
| "To load a classical dataset into a target register of bitsize $b$ and shape\n", | |
| "$\\text{target\\_shape}$, the clifford cost of this QROM scales as\n", | |
| "$\\mathcal{O}(b \\cdot \\text{np.prod(selection\\_shape+target\\_shape)})\n", | |
| "=\\mathcal{O}(b \\cdot \\text{np.prod(data.shape)})$. This is because we need $\\mathcal{O}(b)$\n", | |
| "$\\mathrm{target\\_shape}$, the clifford cost of this QROM scales as\n", | |
| "$\\mathcal{O}(b \\cdot \\mathrm{np.prod(selection\\_shape+target\\_shape)})\n", | |
| "=\\mathcal{O}(b \\cdot \\mathrm{np.prod(data.shape)})$. This is because we need $\\mathcal{O}(b)$\n", | |
| "CNOT gates to load 1 classical data element in the target register and for each of the\n", | |
| "$\\text{np.prod(selection\\_shape)}$ selection indices, we have $\\text{np.prod(target\\_shape)}$\n", | |
| "$\\mathrm{np.prod(selection\\_shape)}$ selection indices, we have $\\mathrm{np.prod(target\\_shape)}$\n", | |
| "all dimensions (i.e. $\\mathcal{O}(\\mathrm{np.prod}(\\mathrm{selection\\_shape}))$).\n", | |
| "\n", | |
| "### Clifford Cost\n", | |
| "To load a classical dataset into a target register of bitsize $b$ and shape\n", | |
| "$\\mathrm{target\\_shape}$, the clifford cost of this QROM scales as\n", | |
| "$\\mathcal{O}(b \\cdot \\mathrm{np.prod}(\\mathrm{selection\\_shape}+\\mathrm{target\\_shape}))$\n", | |
| "$=\\mathcal{O}(b \\cdot \\mathrm{np.prod}(\\mathrm{data.shape}))$. This is because we need $\\mathcal{O}(b)$\n", | |
| "CNOT gates to load 1 classical data element in the target register and for each of the\n", | |
| "$\\mathrm{np.prod}(\\mathrm{selection\\_shape})$ selection indices, we have $\\mathrm{np.prod}(\\mathrm{target\\_shape})$\n" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated to use
\mathrm{}instead of\text{}.Fixes #1667