Skip to content
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

feat(qsystem): add Random number generation module #822

Merged
merged 9 commits into from
Mar 7, 2025
Merged

Conversation

qartik
Copy link
Member

@qartik qartik commented Feb 26, 2025

Closes: #769

Depends on:

Some changes from the issue:

  • Introduces RNG guppy type, instead of the RNGContext guppy struct.
  • We have def __new__(seed: int) -> "RNG": instead of __init__(seed: int) -> "RNGContext"
  • We have maybe_rng instead of init_safe(seed: int) -> Option["RNGContext"]

Other thoughts:

Unsure if this interface is the best we can do:

def test() -> tuple[int, nat, float, int]:
    rng = RNG(42)
    rint = rng.random_int()
    rnat = rng.random_nat()
    rfloat = rng.random_float()
    rint_bnd = rng.random_int_bounded(100)
    rng.discard()
    return rint, rnat, rfloat, rint_bnd

It might be nice to provide a context manager interface such as:

def test() -> tuple[int, nat, float, int]:
    with RNG(42) as rng:
        rint = rng.random_int()
        rnat = rng.random_nat()
        rfloat = rng.random_float()
        rint_bnd = rng.random_int_bounded(100)
    return rint, rnat, rfloat, rint_bnd

which would enforce the linear discipline on RNG. Mark confirms it's currently not possible within guppy.


This comment override removes the release-as flag in the original commit:

BEGIN_COMMIT_OVERRIDE
feat(qsystem): add Random number generation module
END_COMMIT_OVERRIDE

@qartik

This comment was marked as outdated.

@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 94.02985% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.11%. Comparing base (d9b085f) to head (358e85c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
guppylang/std/qsystem/random.py 91.42% 3 Missing ⚠️
guppylang/std/_internal/compiler/arithmetic.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #822      +/-   ##
==========================================
+ Coverage   93.06%   93.11%   +0.05%     
==========================================
  Files          72       74       +2     
  Lines        8564     8633      +69     
==========================================
+ Hits         7970     8039      +69     
  Misses        594      594              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@qartik qartik marked this pull request as ready for review February 27, 2025 19:29
@qartik qartik requested a review from a team as a code owner February 27, 2025 19:29
@qartik qartik requested a review from ss2165 February 27, 2025 19:29
Copy link
Collaborator

@mark-koch mark-koch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!

@qartik qartik added the wait to merge This PR to be merged after all dependencies are ready label Feb 28, 2025
@ss2165
Copy link
Member

ss2165 commented Mar 3, 2025

It's unclear whether init_safe(seed: int) -> Option["RNGContext"] can be written without maintaining any state and whether it's actually useful given the explicit linear typing currently required to use RNG

I don't follow why this is sorry - the __new__ method is unwrapping the option, __init_safe__ would presumably just return it as-is?

@qartik
Copy link
Member Author

qartik commented Mar 3, 2025

I don't follow why this is sorry - the __new__ method is unwrapping the option, __init_safe__ would presumably just return it as-is?

__new__ is a special dunder that's called for memory allocation even before __init__. It returns a new object and hence doesn't require passing in self as its first argument unlike __init__. So apart from __new__, I can't write another method that doesn't require self.

I also tried using @staticmethod decorator for __init_safe__, but then I get:

tests/integration/test_qsystem.py:8: in <module>
    from guppylang.std.qsystem.random import RNG
guppylang/std/qsystem/random.py:26: in <module>
    class RNG:
guppylang/std/qsystem/random.py:34: in RNG
    @guppy(qsystem_random)
guppylang/decorator.py:141: in <lambda>
    return lambda s: dec(s, arg)
guppylang/decorator.py:118: in dec
    return module.register_func_def(f)
guppylang/module.py:229: in register_func_def
    defn = RawFunctionDef(DefId.fresh(self), f.__name__, None, f, get_py_scope(f))
guppylang/module.py:404: in get_py_scope
    code = f.__code__
E   AttributeError: 'staticmethod' object has no attribute '__code__'. Did you mean: '__call__'?

which perhaps suggests that static methods are not yet supported in Guppy?

@ss2165
Copy link
Member

ss2165 commented Mar 3, 2025

ah that's fine (indeed static methods are not supported) - just have it available as a standalone function (new_rng_checked or something like that). I think this just means making your _new_rng_context function public effectively with a docstring and and a less ambiguous name.

@qartik
Copy link
Member Author

qartik commented Mar 3, 2025

Sounds good. Blocked by #810 Also I will need to write a wrapper maybe_rng instead of directly using _new_rng_context as docstrings don't yet work on placeholder functions, to be fixed in #826

@ss2165
Copy link
Member

ss2165 commented Mar 4, 2025

#810 has been closed: abb1aa1

github-merge-queue bot pushed a commit to CQCL/tket2 that referenced this pull request Mar 4, 2025
See CQCL/guppylang#822

BREAKING CHANGE: To be compatible with Guppy's convention of implicitly
returning `self` as the second value of the tuple, the following
signatures are updated:
```diff
-    /// `fn random_int(RNGContext) -> (RNGContext, u32)`
+   /// `fn random_int(RNGContext) -> (u32, RNGContext)`

-   /// `fn random_float(RNGContext) -> (RNGContext, f32)`
+   /// `fn random_float(RNGContext) -> (f32, RNGContext)`

-   /// `fn random_int_bounded(RNGContext, bound: u32) -> (RNGContext, u32)`
+   /// `fn random_int_bounded(RNGContext, bound: u32) -> (u32, RNGContext)`
```
@qartik
Copy link
Member Author

qartik commented Mar 4, 2025

@ss2165 is it accurate that we need CQCL/hugr#1946 and another PR for truncation to be able to finish this PR work on the eldarion PR?

Or am I missing how to perform type conversions between 32-bit and 64-bit ints?

@ss2165
Copy link
Member

ss2165 commented Mar 4, 2025

I think that's right - I didn't realise they weren't being emitted apologies

@qartik qartik force-pushed the 769-add-prng branch 2 times, most recently from c40616b to 348ca54 Compare March 5, 2025 17:55
@qartik qartik requested a review from mark-koch March 6, 2025 14:48
Copy link
Collaborator

@mark-koch mark-koch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good!

@qartik qartik mentioned this pull request Mar 6, 2025
@qartik
Copy link
Member Author

qartik commented Mar 6, 2025

I have a patch to take this to merge, but it might require #846

diff --git a/pyproject.toml b/pyproject.toml
index b5b31b2..5a826c0 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -39,7 +39,7 @@ dependencies = [
     "pydantic >=2.7.0b1,<3",
     "typing-extensions >=4.9.0,<5",
     "hugr >=0.10.3,<0.11",
-    "tket2-exts>=0.4.0,<0.5",
+    "tket2-exts>=0.5.0,<0.6",
 ]

 [project.optional-dependencies]
@@ -65,7 +65,7 @@ test = [
     "pytest-notebook >=0.10.0,<0.11",
     "pytest-snapshot >=0.9.0,<1",
     "ipykernel >=6.29.5,<7",
-    "tket2>=0.6.1",
+    "tket2>=0.7.0",
     "pytest-benchmark>=5.1.0",
 ]
 execution = [
@@ -86,7 +86,7 @@ execute-llvm = { workspace = true }

 # Uncomment these to test the latest dependency version during development
 #  hugr = { git = "https://github.com/CQCL/hugr", subdirectory = "hugr-py", rev = "e40b6c7" }
-tket2-exts = { git = "https://github.com/CQCL/tket2", subdirectory = "tket2-exts", rev = "633ebd7"}
+# tket2-exts = { git = "https://github.com/CQCL/tket2", subdirectory = "tket2-exts", rev = "633ebd7"}
 # tket2 = { git = "https://github.com/CQCL/tket2", subdirectory = "tket2-py", rev = "259cf88"}

@qartik qartik added this pull request to the merge queue Mar 7, 2025
Merged via the queue into main with commit 08fbf47 Mar 7, 2025
3 checks passed
@qartik qartik deleted the 769-add-prng branch March 7, 2025 13:20
@qartik
Copy link
Member Author

qartik commented Mar 20, 2025

ah that's fine (indeed static methods are not supported) - just have it available as a standalone function (new_rng_checked or something like that). I think this just means making your _new_rng_context function public effectively with a docstring and and a less ambiguous name.

Hi @ss2165, I think maybe_rng or init_safe are useless, given there is no failure at runtime on seeding the RNG twice. I am removing them in #873

I recall anticipating this issue in #822 (comment) but we hadn't tested full stack then.

@ss2165
Copy link
Member

ss2165 commented Mar 20, 2025

@qartik The issue is not seeding the rng twice, it is a lack of guarantee that you have a unique handle to rng state, so reproducibility isn't guaranteed because rng calls could be reordered. The failure at runtime should be from calling initialisation twice without discarding first.

@qartik
Copy link
Member Author

qartik commented Mar 20, 2025

@qartik The issue is not seeding the rng twice, it is a lack of guarantee that you have a unique handle to rng state, so reproducibility isn't guaranteed because rng calls could be reordered. The failure at runtime should be from calling initialisation twice without discarding first.

I am unsure which runtime we are referring to. If the platform call doesn't fail, how can we detect this ordering violation?

Since RNG state is a linear type, it's tracked statically by the guppy type checker. The other way I was thinking of implementing maybe_rng was storing some state in RNG type itself, but that also will not be a runtime failure.

github-merge-queue bot pushed a commit that referenced this pull request Mar 21, 2025
For random_nat, see: quantinuum-dev/hugrverse#138

Discussion on maybe_rng at
#822 (comment)

BREAKING CHANGE: removed `random.RNG.random_nat()` and
`random.maybe_rng()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wait to merge This PR to be merged after all dependencies are ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PRNG qsystem lib
4 participants