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

Added DoubleAsStringWithPrecision function - Complex Arithmetics #1883

Merged
merged 6 commits into from
Aug 28, 2024

Conversation

devikamehra
Copy link
Contributor

@devikamehra devikamehra commented Aug 25, 2024

Added DoubleAsStringWithPrecision function in Complex Arithmetics Kata

This is to address the issue #1788

@Manvi-Agrawal
Copy link
Contributor

It might be a good idea to rename the PR title here to include the Kata name. Seeing 2 PR with the same title is confusing. I really got confused by this title because I know JPark1023 worked on adding this function to QDK. Also, it might be helpful to add the link to GitHub issue created by Mariia(tcNickolas) for this.

@devikamehra devikamehra changed the title Added DoubleAsStringWithPrecision function Added DoubleAsStringWithPrecision function - Complex Arithmetics Aug 25, 2024
Copy link
Member

@tcNickolas tcNickolas left a comment

Choose a reason for hiding this comment

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

There is a way to reduce the scope of this change greatly and to make the code more readable. You're passing precision as an argument to the library code, so you have to define it in every task that uses the library. But the precision you use doesn't actually vary between the tasks, and the library not general-purpose but is limited to the needs of this one kata. You can instead hardcode precision within ComplexAsString and ComplexPolarAsString - this way you don't need to change any code outside the library!

Looking at the numbers involved, I don't think we need that much of precision too - none of them are smaller than 1, so 2-3 digits of precision should be plenty.

Thank you!

@devikamehra
Copy link
Contributor Author

There is a way to reduce the scope of this change greatly and to make the code more readable. You're passing precision as an argument to the library code, so you have to define it in every task that uses the library. But the precision you use doesn't actually vary between the tasks, and the library not general-purpose but is limited to the needs of this one kata. You can instead hardcode precision within ComplexAsString and ComplexPolarAsString - this way you don't need to change any code outside the library!

Looking at the numbers involved, I don't think we need that much of precision too - none of them are smaller than 1, so 2-3 digits of precision should be plenty.

Thank you!

Got it @tcNickolas, I have updated the code with changes in only 3 files.

@tcNickolas
Copy link
Member

Can you please resolve conflicts in Common.qs? CI cannot run with them there

@devikamehra
Copy link
Contributor Author

Can you please resolve conflicts in Common.qs? CI cannot run with them there

Resolved

Copy link
Member

@tcNickolas tcNickolas left a comment

Choose a reason for hiding this comment

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

Output looks good, just one suggestion on the resolved conflict.

Thank you!

katas/content/complex_arithmetic/Common.qs Outdated Show resolved Hide resolved
@cesarzc cesarzc added this pull request to the merge queue Aug 28, 2024
Merged via the queue into microsoft:main with commit cf36843 Aug 28, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants