-
Notifications
You must be signed in to change notification settings - Fork 87
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
More accurate solution to kata #1902
base: main
Are you sure you want to change the base?
Conversation
Fix conditional to follow goal
Changed to while loops to ensure theta is within bounds as described in problem
@@ -3,10 +3,10 @@ namespace Kata { | |||
|
|||
function ComplexPolarMult(x : ComplexPolar, y : ComplexPolar) : ComplexPolar { | |||
mutable theta = x.Argument + y.Argument; | |||
if theta > PI() { | |||
while theta > PI() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I think that this task assumes the given complex numbers x and y have −π < θ ≤ π , which is the principal argument. That's why if condition is used. Although, QDK in itself doesnt throw an error if one provides argument θ, which doesnt follow these bounds. That being said there is definitely room for improvement.
Quoting verbation about "principal argument" from "5.6 Argument of a complex number" from [Elementary Linear Algebra by Keith Matthews](http://www.numbertheory.org/book/)
"Let z = x + iy be a non–zero complex number, r = |z| = \sqrt{x^2+y^2}. Then
we have x = r cos θ, y = r sin θ, where θ is the angle made by z with the
positive x–axis. So θ is unique up to addition of a multiple of 2π radians.
Any number θ satisfying the above pair of equations is called an argument of z and is denoted by arg z. The particular argument of z lying in the range −π < θ ≤ π is called the principal
argument of z and is denoted by Arg z (see Figure 5.5)."
Approach 1
I would suggest to improve the "Polar Cordinates" lesson to include this note about the principal argument of a complex number and subsequent task descriptions to include this information. For example, "Polar multiplication" task could be rephrased as :
- A complex polar number
$x = r_{1}e^{i\theta_1}$ in its principal form, i.e.$r_1 \geq 0$ and$-\pi < \theta_1 \leq \pi$ . - A complex polar number
$y = r_{2}e^{i\theta_2}$ in its principal form, i.e.$r_2 \geq 0$ and$-\pi < \theta_2 \leq \pi$ ..
Approach 2
Keep this solution. Change testing strategy to explicitly generate the magnitude and theta. Then, update testing strategy to fail the complex numbers where someone has used if
condition instead of while
loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking about edge case testing but more from a brute force angle to find if there was one. I never quite got the hang of trigonometry and spent way too long trying to somewhat understand this lesson/solution. Really appreciate you including links where the problems are discussed in more detail!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically, the addition/subtraction of
Its just the thing that for a complex number
PS: [
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree with the first approach @Manvi-Agrawal suggested, where the fix will be in text for the lesson to clarify the inputs are expected within a specific range. While this solution is more generally correct, I don't think updating the tests to fail user solutions that don't accommodate values outside the range is that helpful for what is meant to be an introductory task.
@cesarzc, @tcNickolas any preference here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree with the first approach. Changing test should be done carefully since users' solutions persist in the Azure Quantum katas environment. If users start getting error from previously accepted solutions, that results in a bad experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree the with approach 1 being a better solution, my confusion was due to not understanding the concepts behind the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HopeAnnihilator, I was wondering if you would have some time to do the changes to this PR to align with the agreed approach? Is there anything else we can help you with?
More accurate solution for Polar Multiplication Kata