Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@msoeken
Copy link
Member

@msoeken msoeken commented Feb 1, 2021

This PR fixes #403 by replacing the recursive implementation with an iterative one. However, reading @vadym-kl's issue description I wonder whether it is better to implement it as a Q# intrinsic and calling directly into C#'s method to retrieve the bit size.

@msoeken msoeken requested a review from cgranade February 1, 2021 08:19
@cgranade
Copy link
Contributor

cgranade commented Feb 2, 2021

It looks like the build failure is transient, so maybe let's kick off another one?

/azp run

With respect to the other point, I think especially for QIR emission, it would make sense to keep this backed by your Q# implementation so as to not introduce another intrinsic that target packages need to implement.


@Test("QuantumSimulator")
function CanComputeBitSizeFromLargeNumbers () : Unit {
for (k in 1 .. 100) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (k in 1 .. 100) {
for k in 1 .. 100 {

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the current libraries still builds against an older version of the QDK.

return AccumulatedBitsizeL(a, 0);
mutable bitsize = 0;
mutable val = a;
while (val != 0L) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while (val != 0L) {
while val != 0L {

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the current libraries still builds against an older version of the QDK.

msoeken and others added 2 commits February 2, 2021 17:24
Co-authored-by: Chris Granade <chgranad@microsoft.com>
@msoeken
Copy link
Member Author

msoeken commented Feb 2, 2021

With respect to the other point, I think especially for QIR emission, it would make sense to keep this backed by your Q# implementation so as to not introduce another intrinsic that target packages need to implement.

Can we do both? Provide a Q# implementation but also have a C# implementation for when we run Q# applications inside C#? I think such an approach would be generally helpful.

@cgranade
Copy link
Contributor

cgranade commented Feb 2, 2021

Can we do both? Provide a Q# implementation but also have a C# implementation for when we run Q# applications inside C#? I think such an approach would be generally helpful.

Good question, yeah. @swernli, I know you've raised that some of the behavior around that has changed recently with the move to targeting packages; how would be best to have the best of both worlds here?

@swernli
Copy link
Contributor

swernli commented Feb 2, 2021

Thanks for drawing my attention to this, Chris! The good news is that the target package work (which merged to qsharp-runtime/main as of microsoft/qsharp-runtime#476) was able to accomplish its goals in a (mostly) non-breaking way. So the behavior I raised before that changed actually didn't change and can still work as it did before. For clarity, here's a list of the current behaviors you can use:

  • If you provide a Q# implementation for a callable and a C# implementation, the C# implementation will override the Q# as it does today, using the existing mechanisms for discovering child classes in the C# and registering them as overrides their parent (this behavior was going to change but now has not).
  • If you define a Q# callable as body intrinsic then by default it will be generated into an abstract C# class, so failing to provide a C# child class with implementation will result in a runtime failure because the infrastructure won't find any overrides and will try to instantiate the abstract class (also matches current behavior).
  • If you define a Q# callable as body intrinsic and opt-in to the the new behavior it will be generated as a concrete C# class that calls into an interface to invoke the desired implementation. You must then define the interface implement it on your desired simulator or other runtime environment to get the right functionality, and will get compile time errors if the interface definition is missing and runtime errors if the chosen simulator doesn't implement that interface (this is the new behavior and is entirely optional).

So it sounds like in this case you want to use that first option, which will still work as it does today. That way the Q# implementation will be used when generating code (such as for QIR) but the C# implementation will take precedence with our default simulator.

@cgranade
Copy link
Contributor

cgranade commented Feb 2, 2021

Thanks for the help, @swernli!

  • If you provide a Q# implementation for a callable and a C# implementation, the C# implementation will override the Q# as it does today, using the existing mechanisms for discovering child classes in the C# and registering them as overrides their parent (this behavior was going to change but now has not).

So it sounds like in this case you want to use that first option, which will still work as it does today. That way the Q# implementation will be used when generating code (such as for QIR) but the C# implementation will take precedence with our default simulator.

Agreed, I think this meets both usecases fairly nicely. It's good to have the version that relies on the nicely optimized code from the C# standard libraries while still being able to use the pure Q# version in QIR. My only concern at that point would be testing to make sure that the Q# version is correct when the C# implementation is missing; I think that isn't quite covered by @Test(...) at the moment, such that we may need a bit more there...

@msoeken
Copy link
Member Author

msoeken commented Feb 3, 2021

Thanks for the help and feedback @swernli and @cgranade. I am also not clear at the moment, how we can test both implementations. I will merge this PR for now and eventually open another one, adding a C# native implementation when we sorted out the test questions.

@msoeken msoeken merged commit 472f812 into main Feb 3, 2021
@msoeken msoeken deleted the msoeken/bitsizel branch February 3, 2021 07:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stack overflow when using Microsoft.Quantum.Math.BitSizeL

4 participants