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

Turned exercise 039 into practice #2281

Merged
merged 9 commits into from
Mar 29, 2024
Merged

Conversation

jahielkomu
Copy link
Contributor

@jahielkomu jahielkomu commented Mar 20, 2024

Addresses #2200. Turns Exercise 039 into Practice
@cuihtlauac please kindly review

@divyankachaudhari
Copy link
Contributor

divyankachaudhari commented Mar 21, 2024

@jahielkomu I was working on the issue you've linked :) Also, you've linked Exercise 037 in the link, rather than 039.

let rest = all_primes (a + 1) b in
if is_prime a then a :: rest else rest


Copy link
Contributor

Choose a reason for hiding this comment

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

Remove spaces at the end of files, add new line.

@@ -0,0 +1,41 @@

Copy link
Contributor

@divyankachaudhari divyankachaudhari Mar 21, 2024

Choose a reason for hiding this comment

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

Remove space at the start and end of all files.

module Make(Tested: Testable) : sig val v : test end = struct
open Tested

let is_prime_tests = "is_prime" >::: [
Copy link
Contributor

Choose a reason for hiding this comment

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

This question only asks for all_primes and not is_prime

open OUnit2

module type Testable = sig
val is_prime : int -> bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove signature of is_prime.

@@ -0,0 +1,2 @@
let prime_tests _ = failwith "Not yet implemented"
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be all_primes function

@jahielkomu
Copy link
Contributor Author

@jahielkomu I was working on the issue you've linked :) Also, you've linked Exercise 037 in the link, rather than 039.

Hello @divyankachaudhari , sorry I thought you were working on Issues #2278 and #2211
I thought it would be okay to join you on this one.

Thank you for pointing out the error in linkage

@jahielkomu
Copy link
Contributor Author

Hello @divyankachaudhari
I had figured that to first assert is_prime works correctly would be important then proceed to assert all_primes.

Please I have updated the PR.
Kindly review and revert.
Thanks.

@divyankachaudhari
Copy link
Contributor

divyankachaudhari commented Mar 25, 2024

@jahielkomu Yes, is_prime does need to work properly. But, the question asks for all_primes only. Now, a learner may implement it in any way that they want to. So, is_prime is not a part of question; hence it needn't be checked. You don't have to make test cases according to the model answer in the answer folder. You have to make it for the function you put as "Not yet Implemented" in the work folder.

@jahielkomu
Copy link
Contributor Author

jahielkomu commented Mar 25, 2024

@jahielkomu Yes, is_prime does need to work properly. But, the question asks for all_primes only. Now, a learner may implement it in any way that they want to. So, is_primes is not a part of question, hence it needn't be checked. You don't have to make test cases according to the model answer in the answer folder. You have to make it for the function you put as "Not yet Implemented" in the work folder.

I do understand what you are saying @divyankachaudhari
Please kindly review the updates to the PR concerning ' Turning exercise 039 into Practice '
Thank you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be better tested if the all the numbers in the returned list are tested to be actually prime or not. Take inspiration from PR #2236.

  • Inline a list of primes in ex.ml
  • Turn the specified range into a value of type (int * bool) list
  • Turn that latter list into test cases, using List.map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @cuihtlauac ,
I have indeed gained inspiration from PR #2236.

I however would like to say if a prime number doesn't fall in the list I inline then I may label it not prime which won't be correct because a prime can fall outside the primes I inline. I hope I understood your review correctly.

Solutions I'm thinking of:

  • Listing all the primes from 2 to 7920 and pattern matching on the list to generate tuples.
  • Checking each number directly and then generating tuples..(this would mean including a function that checks if a number is prime)

What is your say @cuihtlauac ?

@divyankachaudhari if there is anything you can point out, please do so too.

Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

PR #2236 is now mergved into folder 031. Take inspiration from that to test all the values in the range

@cuihtlauac cuihtlauac added the outreachy Outreachy contributions and blog posts label Mar 25, 2024
@cuihtlauac
Copy link
Collaborator

@jahielkomu are you still working on this?

@jahielkomu
Copy link
Contributor Author

@jahielkomu are you still working on this?

Yes @cuihtlauac I am.
I had tagged you in my previous comment hoping to pick your mind sir.

Copy link
Collaborator

@cuihtlauac cuihtlauac left a comment

Choose a reason for hiding this comment

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

Fix end of files, none finishing with newline character

@jahielkomu
Copy link
Contributor Author

Hello @cuihtlauac, I hope you are well.

I have updated the ex.ml file of practice/039 but solely to pick your mind.
I'm not very confident about these changes thus I kindly ask for your guidance.

Workflow.
-Have a function that checks if a number is prime
-Have another one that creates a list of type (int * bool) list

It is after this stage that I get a bit confused.
As I map through the the list what is supposed to happen?

@jahielkomu
Copy link
Contributor Author

jahielkomu commented Mar 28, 2024

Hello @cuihtlauac I hope you are well.
Please review the updates to the PR.
I'm I on the right track?
Thanks

practice/039/answer/impl.ml Outdated Show resolved Hide resolved
practice/039/answer/impl.ml Outdated Show resolved Hide resolved
practice/039/ex.ml Outdated Show resolved Hide resolved
practice/039/ex.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@cuihtlauac cuihtlauac left a comment

Choose a reason for hiding this comment

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

I've fixed the PR. Thanks @jahielkomu

@cuihtlauac cuihtlauac merged commit 90d0675 into ocaml:main Mar 29, 2024
2 of 3 checks passed
@jahielkomu
Copy link
Contributor Author

I've fixed the PR. Thanks @jahielkomu

Thanks for the assistance @cuihtlauac .
If it is okay I would love to followup and ask, what happens if a user's input is a range above 200?

@cuihtlauac
Copy link
Collaborator

cuihtlauac commented Mar 29, 2024

Program testing can be used to show the presence of bugs, but never to show their absence!

If the input is above 200, it's just an untested value.

@jahielkomu
Copy link
Contributor Author

Program testing can be used to show the presence of bugs, but never to show their absence!

If the input is above 200, it's just an untested value.

This makes sense @cuihtlauac .
Thanks.

Are there more issues we can work on as we delve deeper into Ocaml?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
outreachy Outreachy contributions and blog posts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants