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

"Hamming" Exercise allows solutions that produce incorrect results #285

Closed
bomber34 opened this issue Jan 2, 2024 · 1 comment
Closed

Comments

@bomber34
Copy link

bomber34 commented Jan 2, 2024

The following code passes the test cases but is not correct.

hamming_distance_chars([], [], 0).
hamming_distance_chars([C|Cs1], [C|Cs2], Dist) :- 
    hamming_distance_chars(Cs1, Cs2, Dist).
hamming_distance_chars([C1|Cs1], [C2|Cs2], Dist) :- 
    hamming_distance_chars(Cs1, Cs2, SubDist),
    Dist is SubDist + 1.

hamming_distance(Str1, Str2, Dist) :-
    string_length(Str1, Len1),
    string_length(Str2, Len2),
    Len1 == Len2,
    string_chars(Str1, Chars1),
    string_chars(Str2, Chars2),
    hamming_distance_chars(Chars1, Chars2, Dist).

The issue lies in the hamming_distance_chars/3 predicate. The third case does not check if C1 and C2 are different.
Therefore for the call
?- hamming_distance("AAA", "AAA", Result).
We get the following answers:

Result = 0
Result = 1
Result = 1
Result = 2
Result = 1
Result = 2
Result = 2
Result = 3

However, the only valid answer should be 0.
The root cause seems to stem from the attempt of fixing the non-exhaustive test cases as raised in issue #81 since the test cases behave no different than prolog itself
test(long_identical_strands, condition(pending)) :- hamming_distance("GGACTGA", "GGACTGA", Result), Result == 0.
As long as hamming_distance/3 is able to find a Result which is equal to 0, the test passes.

In my opinion, the better method to write test cases in prolog is to first consider if the exercise expects a deterministic solution (exactly one or zero solutions for bound inputs) or a non-deterministic solution (can have multiple solutions for bound input variables).

Therefore, instead of trying to bind Result and then check if there was a found answer matching our expectation, I suggest using the -Options argument in the test/2 predicate. That should make the intention of the tests clear and avoid the issues in #81

:- begin_tests(hamming).
    % problematic as pointed out in issue #81 and does not invalidate wrong solutions
    test(identical_strands, condition(true)) :-
        hamming_distance("A", "A", 0).

    % accepts only one solution
    test(det_long_identical_strands, true(Result =:= 0)) :-
        hamming_distance("GGACTGA", "GGACTGA", Result).

    % if multiple solutions were considered valid for this exercise for the provided code at the start of this issue
    test(nondet_long_identical_strands, set(Result == [0,1,2,3,4,5,6,7])) :-
        hamming_distance("GGACTGA", "GGACTGA", Result).

:- end_tests(hamming).

While I am sure this is a general problem on this exercism track, I noticed it during a mentoring session for this exercise in particular.

Copy link

github-actions bot commented Jan 2, 2024

Hello. Thanks for opening an issue on Exercism 🙂

At Exercism we use our Community Forum, not GitHub issues, as the primary place for discussion. That allows maintainers and contributors from across Exercism's ecosystem to discuss your problems/ideas/suggestions without them having to subscribe to hundreds of repositories.

This issue will be automatically closed. Please use this link to copy your GitHub Issue into a new topic on the forum, where we look forward to chatting with you!

If you're interested in learning more about this auto-responder, please read this blog post.

@github-actions github-actions bot closed this as completed Jan 2, 2024
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

No branches or pull requests

1 participant