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

Handle numerical issues complex_numbers_spec.cr #528

Merged
merged 2 commits into from
Oct 16, 2023

Conversation

f-fr
Copy link
Contributor

@f-fr f-fr commented Oct 15, 2023

The spec uses eq to test for equality of complex numbers. However, the real and imaginary parts are Float64, hence there might be numerical issues. Instead, the be_close expectation should be used.

Using eq would require the Complex class to be modified (e.g. by overloading == to be slightly imprecise). But this would somehow hard-code the precision of Complex, which would not be a good idea if Complex was a "production" implementation. Although it is only an exercise, it should not encourage "bad style" like this.

The spec uses `eq` to test for equality of complex numbers. However, the real and imaginary parts are `Float64`, hence there might be numerical issues. Instead, the `be_close` expectation should be used.
@ryanplusplus
Copy link
Member

The changes look good but they need to be done in the test template because the tests aren't generated instead of manually written.

Copy link
Member

@ryanplusplus ryanplusplus left a comment

Choose a reason for hiding this comment

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

Make these changes in exercises/practice/complex-numbers/.meta/test_template.ecr

@f-fr
Copy link
Contributor Author

f-fr commented Oct 16, 2023

I could also modify the example solution and remove the == operator (it is not needed anymore).

@ryanplusplus
Copy link
Member

I could also modify the example solution and remove the == operator (it is not needed anymore).

Yes please

@ryanplusplus ryanplusplus merged commit 2803b5a into exercism:main Oct 16, 2023
8 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.

3 participants