Add QGF data type for Galois Fields #1433
Conversation
|
Ugh, looks like both galois and quimb use numba and that's causing issues with notebook tests that I'm unable to reproduce locally. @mpharrigan Do you have any idea how to fix this? |
|
usually depending on the same package would be a boon for compatibility! I'll investigate |
|
From a brief look, I think quimb setting the number of numba threads explicitly may be causing the issue: https://github.com/jcmgray/quimb/blob/8ecf9d6ef7d2055ebbae5f7dbb5aeb0e65c0d422/quimb/core.py#L37-L51 Numba also has a default but one uses I couldn't find anything in galois that explicitly sets number of threads for numba, but maybe there's some sneaky logic living somewhere. Thanks for looking into this and let me know what you find! |
|
can't reproduce locally, but seems to be https://github.com/nasa/hybridq/blob/main/README.md#runtimeerror-cannot-set-numba_num_threads-to-a-different-value-once-the-threads-have-been-launched |
|
I guess the question is who is changing the NUMBA_NUM_THREADS after its precached, and galois is probably doing that given the failures are seen in this PR. |
|
I tried just importing the packages, which didn't trigger the bug. I tried disabling the parallel execution of notebooks which didn't fix the bug. From the error message and synthesizing some of the info from @tanujkhattar 's investigation above: it looks like the original value of 2 is incorrect if we're trying to set it to the number of cores, which github documents as 4 https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories which is the second value |
|
How about we patch the CI failure by explicitly setting the environment variable (see current status) and keep an eye out for this showing up on an actual computer in a reproducible way. |
|
Thanks, that sounds like a good plan to me |
|
@mpharrigan Since all tests are passing now, can you please take a look at the PR? |
| degree: The degree $m$ of the field $GF(p^{m})$. The degree must be a positive integer. | ||
|
|
||
| References | ||
| [Finite Field](https://en.wikipedia.org/wiki/Finite_field) |
There was a problem hiding this comment.
how does this get rendered in the api docs? Usually we separate individual references with a blank newline; but I admit that that is primarily for the notebook-based doc system
There was a problem hiding this comment.
added blank newline between references.
qualtran/_infra/data_types.py
Outdated
| A Finite Field or Galois Field is a field that contains finite number of elements. The order | ||
| of a finite field is the number of elements in the field, which is either a prime number or | ||
| a prime power. For every prime number $p$ and every positive integer $m$ there are fields of | ||
| order $p^{m}0$, all of which are isomorphic. When m=1, the finite field of order p can be |
There was a problem hiding this comment.
is this 0 supposed to be here? what does it mean?
There was a problem hiding this comment.
was a typo, removed.
qualtran/_infra/data_types.py
Outdated
| as the irreducible polynomial. | ||
|
|
||
| Attributes: | ||
| characteristic: The characteristic $p$ of the field $GF(p^{m})$. |
There was a problem hiding this comment.
personal preference, but I generally remove rendundant latex { }. You can just write $p^m$.
qualtran/simulation/classical_sim.py
Outdated
| @@ -81,16 +81,23 @@ def _numpy_dtype_from_qdtype(dtype: 'QDType') -> Type: | |||
| return object | |||
|
|
|||
|
|
|||
| def _numpy_empty_reg_from_reg(reg: Register) -> np.ndarray: | |||
There was a problem hiding this comment.
"nump" and "from reg" makes sense; but we're not returning an empty register per se. _numpy_empty_array_from_reg? _np_empty_from_reg? _empty_ndarray_from_reg?
There was a problem hiding this comment.
Should this function be an overridable method on QDType? Not blocking this PR but if you think it is a good idea we can open an issue
There was a problem hiding this comment.
Yes, I think it's a good idea to have this function on then QDType. It would also make Fxp integration better I think; since Fxp also derives from numpy array and we don't make use of that. This is also why Fxp is slow for us I think.
I'll open an issue.
There was a problem hiding this comment.
Did you end up opening one? There's something during review of #1436 that I'd like to note on the issue as well
| @@ -53,6 +53,7 @@ | |||
| QUInt, | |||
| BQUInt, | |||
| QMontgomeryUInt, | |||
| QGF, | |||
There was a problem hiding this comment.
any concerns about effects on import time? I know nothing will be slower than quimb but you can theoretically avoid importing quimb sometimes
There was a problem hiding this comment.
Moved from galois import GF to a local import inside a method of QGF. Verified that time python -c 'import qualtran' has no degradation from this PR now.
|
Thanks for the review, I've addressed the nits. Merging now. |
Part of #1419
Serialization support for new QDType will be added in a follow up PR