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

Add exponent tests for complex-numbers #64

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mbramson
Copy link
Contributor

@mbramson mbramson commented Jun 7, 2018

The complex-numbers exercise instructs users to implement the exponent function in the README, but there are no tests for this function and no function stub. As a result, no users have yet implemented this function.

I added 5 tests for the exponent function as well as an exponent stub function to hopefully better direct users to complete the exercise.

Some commentary on the process of coming up with this solution. I ran into a few cases where I got the warning "test succeeded with choicepoint". This sent me down a rabbit hole or two which might lead some who are on the second exercise to get confused. That said, I'm 3 days into prolog at this point, and haven't done any of the later exercises, and so am not sure how the complexity will scale as I get further in. That might be just fine, or that might indicate that this function was too difficult for this exercise, I lack the perspective to say at this point.

@Average-user
Copy link
Member

test succeeded with choicepoint

That happens when you don't use the "!" operator, to end a search point.

@qjd2413
Copy link
Contributor

qjd2413 commented Jun 9, 2018

@Average-user is that something we could test for? I'm not sure if it's possible, but it would be useful to look into.

with regards to the actual content of the PR, the test logic isn't correct-- the first test with X \= 1 returns true because X = 1.0. Something about floating point arithmetic, probably.

@Average-user
Copy link
Member

Average-user commented Jun 11, 2018

with regards to the actual content of the PR, the test logic isn't correct-- the first test with X = 1 returns true because X = 1.0. Something about floating point arithmetic, probably.

You should use =\=:

?- 1.0 \= 1.
true.

?- 1.0 =\= 1.
false.

@qjd2413
Copy link
Contributor

qjd2413 commented Jun 11, 2018

Well I mean more that, with a correct exponent function, X should equal 1 (ignoring floating point), but these tests assert that X doesn't equal 1

@Average-user
Copy link
Member

The complex-numbers exercise instructs users to implement the exponent function in the README, but there are no tests for this function and no function stub. As a result, no users have yet implemented this function.

@mbramson there are tests for the exponent function. Please follow them.

@mbramson
Copy link
Contributor Author

@Average-user I didn't even know that https://github.com/exercism/problem-specifications/blob/master/exercises/complex-numbers/canonical-data.json was a thing! I just came up with those test cases myself. I'll update the PR with those test cases.

With regard to

with regards to the actual content of the PR, the test logic isn't correct-- the first test with X = 1 returns true because X = 1.0. Something about floating point arithmetic, probably.

You should use ==:

?- 1.0 \= 1.
true.

?- 1.0 =\= 1.
false.

I'm not sure what you're saying I should do. The first case is likely the one we want right?

@mbramson
Copy link
Contributor Author

@Average-user @qjd2413 Updated with new tests. Let me know what you think. Still up to change to =/= if you think that leads to a better test.

@qjd2413
Copy link
Contributor

qjd2413 commented Jun 27, 2018

Let's take the first test, for example, exponent((0,pi), (X,Y)), X \=-1, Y \= 0. This is the result of evaluating just exponent((0,pi), (X,Y))
image
Because of floating point arithmetic, Y is not exactly 0, which is why these tests pass. I was incorrect about using == (because it doesn't represent unification), but we should be declaring equality-- \= and=\= declare inequality. Thus, a solution like

exponent((R,I), (Rr,Ir)) :-
  Rr is R + 1, Ir is I + 1.

passes your tests. I can't find any official documentation (or even unofficial) about the way to compare floats, but rationalize(X) =:= rationalize(<a>), rationalize(Y) =:= rationalize(<b>). seems to work for me.

@mbramson
Copy link
Contributor Author

mbramson commented Jul 1, 2018

Oh wow. Well I feel stupid. I totally didn't realize that \= declared inequality.

@mbramson
Copy link
Contributor Author

mbramson commented Jul 1, 2018

Okay, so the tests are actually testing things properly now, (although not exactly precisely, but up to three significant digits of precision). I don't love these tests (obviously infinitely better than before), but I couldn't find a good comparison operator that allowed for success when things were equal within some delta, so I basically wrote that myself.

As a result, these tests aren't as readable as they might be, but they are correct.

Let me know what you think @Average-user and @qjd2413

@mbramson
Copy link
Contributor Author

mbramson commented Jul 1, 2018

As you pointed out =:= does not work here because of floating point arithmetic. I could use the exact floats that I get out of my solution, but that's assuming that everyone's solution will have the exact same amount of float driven error. I think that's a bad assumption as anyone else who encounters such a deviation will have a pretty bad debugging experience as a result.

Now these tests actually test results properly.
@mbramson mbramson force-pushed the add-exponent-tests-for-complex-numbers branch from f67f20c to beb898d Compare July 1, 2018 23:17
@qjd2413
Copy link
Contributor

qjd2413 commented Jul 4, 2018

Could you create a floatEqual rule that looks something like

floatEqual(X, Y, Precision):- X < Y + Precision, X > Y + Precision.

so the tests are easy to understand on first glance?

Base automatically changed from master to main January 28, 2021 19:19
@ErikSchierboom
Copy link
Member

@mbramson Are you interested in getting this PR over the line?

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