Skip to content

Collatz conjecture #545

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

Merged
merged 15 commits into from
Jul 28, 2025
Merged

Collatz conjecture #545

merged 15 commits into from
Jul 28, 2025

Conversation

TheRealOwenRees
Copy link
Contributor

@TheRealOwenRees TheRealOwenRees commented Jul 23, 2025

  • Ported the exercise Collatz Conjecture to OCaml.
  • Added the add-practice-exercise script to generate new practice exercise

Issues:

  • environment does not build when following the readme file, with package conflicts
  • https://exercism.org/docs/tracks/ocaml/installation works, but then some exercises do not run tests due to missing packages that require base compiler v5.1.0
  • in hindsight, using configlet would have been better to create the exercise

@BethanyG BethanyG requested a review from kahgoh July 23, 2025 22:47
@BethanyG
Copy link
Member

@kahgoh - I triggered the CI and gave a once-over to the files, which look fine to my eye.

But the ocaml code needs a more experienced once-over than I can really give it.

Looks like for PR #539, you were talking about making changes to the test generator. Would that also be something you would want here? If so, I think @TheRealOwenRees is more than willing, but would need some guidance on the details. 😄

Copy link
Member

@kahgoh kahgoh left a comment

Choose a reason for hiding this comment

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

Thanks for raising this PR to add collatz conjecture for the OCaml track! I've got some comments below 👇🏻

@TheRealOwenRees
Copy link
Contributor Author

TheRealOwenRees commented Jul 24, 2025

Thanks for raising this PR to add collatz conjecture for the OCaml track! I've got some comments below 👇🏻

Hello and thank you for your comments. I will look into those and resolve those issues.

However I cannot run the commands in the Readme as the OCaml environment does not build. I have a local switch for this and running compiler 5.1.0, as well as trying 5.1.1. I get the following errors:

[ERROR] Package conflict!
  * No agreement on the version of ocaml:
    - (invariant) → ocaml-base-compiler = 5.1.1 → ocaml = 5.1.1
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0
    You can temporarily relax the switch invariant with `--update-invariant'
  * No agreement on the version of ocaml-base-compiler:
    - (invariant) → ocaml-base-compiler = 5.1.1
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0 → ocaml-base-compiler (= 4.08.1 | = 4.11.1)
  * Incompatible packages:
    - (invariant) → ocaml-base-compiler = 5.1.1
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0 → ocaml-system >= 4.02.4
  * Incompatible packages:
    - (invariant) → ocaml-base-compiler = 5.1.1
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0 → ocaml-variants (< 4.05.2~ | >= 4.08.1)
  * Missing dependency:
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0 → ocaml-variants >= 4.08.1 → ocaml-beta
    unmet availability conditions: 'enable-ocaml-beta-repository'
  * Missing dependency:
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0 → ocaml-variants >= 4.08.1 → system-msvc
    unmet availability conditions: 'os = "win32"'

No solution found, exiting

I have also tried with the --update-invariant option.

If you have any pointers to get this working, I am certain I can finalise this PR.

Thanks

@TheRealOwenRees
Copy link
Contributor Author

Thanks for raising this PR to add collatz conjecture for the OCaml track! I've got some comments below 👇🏻

Hello and thank you for your comments. I will look into those and resolve those issues.

However I cannot run the commands in the Readme as the OCaml environment does not build. I have a local switch for this and running compiler 5.1.0, as well as trying 5.1.1. I get the following errors:

[ERROR] Package conflict!
  * No agreement on the version of ocaml:
    - (invariant) → ocaml-base-compiler = 5.1.1 → ocaml = 5.1.1
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0
    You can temporarily relax the switch invariant with `--update-invariant'
  * No agreement on the version of ocaml-base-compiler:
    - (invariant) → ocaml-base-compiler = 5.1.1
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0 → ocaml-base-compiler (= 4.08.1 | = 4.11.1)
  * Incompatible packages:
    - (invariant) → ocaml-base-compiler = 5.1.1
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0 → ocaml-system >= 4.02.4
  * Incompatible packages:
    - (invariant) → ocaml-base-compiler = 5.1.1
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0 → ocaml-variants (< 4.05.2~ | >= 4.08.1)
  * Missing dependency:
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0 → ocaml-variants >= 4.08.1 → ocaml-beta
    unmet availability conditions: 'enable-ocaml-beta-repository'
  * Missing dependency:
    - getopts → lemonade >= 0.2.1 → mixture < 0.2.1 → ocaml < 5.0.0 → ocaml-variants >= 4.08.1 → system-msvc
    unmet availability conditions: 'os = "win32"'

No solution found, exiting

I have also tried with the --update-invariant option.

If you have any pointers to get this working, I am certain I can finalise this PR.

Thanks

I believe to have fixed this. I will submit a PR for troubleshooting / installation guide updates another time, after finalising this PR.

@TheRealOwenRees
Copy link
Contributor Author

TheRealOwenRees commented Jul 25, 2025

  • removed mistakenly added files
  • added template to test generator

I will need to write a special case for the test, where result is a return type with Ok and Error? I think that might be a little beyond me at this stage. I will attempt it and submit again.

@TheRealOwenRees
Copy link
Contributor Author

Test template added.

However it is generating multiple tests for 0 and -15 according to what is in the canonical data:

 {
      "uuid": "7d4750e6-def9-4b86-aec7-9f7eb44f95a3",
      "description": "zero is an error",
      "property": "steps",
      "input": {
        "number": 0
      },
      "expected": {
        "error": "Only positive numbers are allowed"
      }
    },
    {
      "uuid": "2187673d-77d6-4543-975e-66df6c50e2da",
      "reimplements": "7d4750e6-def9-4b86-aec7-9f7eb44f95a3",
      "description": "zero is an error",
      "comments": ["Collatz Conjecture holds only for positive integers"],
      "property": "steps",
      "input": {
        "number": 0
      },
      "expected": {
        "error": "Only positive integers are allowed"
      }
    },
    ```
    
 If someone can point me in the direction to fix that, I believe I may have finally cracked it.
 
 Thanks

@kahgoh
Copy link
Member

kahgoh commented Jul 27, 2025

The test generator doesn't handle the reimplements 😅. Admittedly, this is also causing some of the broken test generation issues.

If you're up fixing this limitation that would be greatly appreciated 😁. You'll probably need to dive into the test-generator code. I think the points of interest are:

  • bin_test_gen/test_gen.ml - the generator's starting point
  • lib_generator/controller.ml - the controller used by test_gen to apply the templates
  • lib_generator/exercism.ml - the of_candidate function is where the canonical data for the exercise is read

If not, that's okay too 😉 . In this case, let's just mark the test generation broken by adding a templates/collatz-conjecture/.broken file. Contents can be empty or a note about not handling the reimplements key.

@kahgoh
Copy link
Member

kahgoh commented Jul 27, 2025

Also, Github is reporting a merge conflict on config.json. Would you mind merging and resolving?

@TheRealOwenRees
Copy link
Contributor Author

  • filter added to remove any UUID tests cases that are referenced by reimplements
  • the above filter affected two other exercises - anagram and pangram
  • resolved config.json conflict

Copy link
Member

@kahgoh kahgoh left a comment

Choose a reason for hiding this comment

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

It is looking good! A couple of minor comments.

.gitignore Outdated
_opam

node_modules
Copy link
Member

Choose a reason for hiding this comment

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

I think you've accidentally added node_modules? Could you please remove.


(* filter original cases whose UUID is referenced in "reimplements" *)
let reimplemented_uuids = get_reimplemented_uuids cases in
let filtered_cases =
Copy link
Member

Choose a reason for hiding this comment

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

The alignment seems a little off. Should this be indented?

Copy link
Member

Choose a reason for hiding this comment

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

This file has been accidentally added again 😅.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dear, this is why I added that line to the .gitignore. My environment likes to add a debug file for some reason. I will delete and resubmit. Apologies for the inconvenience!

Copy link
Member

@kahgoh kahgoh Jul 28, 2025

Choose a reason for hiding this comment

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

Actually, I was kinda curious what might be causing it be generated and added .... At least in my environment, I haven't seen node_modules get generated, although my OCaml version is 4.14.2...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am running a 5.1.1 global switch for this as the Readme stipulates OCaml 5.1 is needed. Perhaps this is the cause of the issue!

Copy link
Member

Choose a reason for hiding this comment

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

Readme stipulates OCaml 5.1 is needed

Oh that's interesting! I had my environment set up a long time ago based on the track docs which says 4.08.0 😅. We should fix that - I'll add that as a separate issue later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually going to submit a PR for this later in the week! I would be interested to see if you have any dependency issues when installing this according to the readme. I had the compiler version errors listed in an earlier comment here, but have a fix. I just need to make sure it is not just me with the error !

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I've just created #546 to note this version discrepancy.

Copy link
Member

@kahgoh kahgoh left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good!

@kahgoh kahgoh added the x:rep/large Large amount of reputation label Jul 28, 2025
@kahgoh kahgoh merged commit 20c62f0 into exercism:main Jul 28, 2025
2 checks passed
@TheRealOwenRees TheRealOwenRees deleted the collatz-conjecture branch July 28, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:rep/large Large amount of reputation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants