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

Sum of multiples #560

Merged
merged 11 commits into from
Mar 13, 2023
Merged

Sum of multiples #560

merged 11 commits into from
Mar 13, 2023

Conversation

colinleach
Copy link
Contributor

runtests.jl and example.jl should be OK, maybe also config.json. tests.toml is blank.

This is a newbie submission, so please advise on what I should do in future to make the reviewer's life easier!

@colinleach colinleach requested a review from a team as a code owner March 12, 2023 14:40
@colinleach
Copy link
Contributor Author

Apparently I should have run new-exercise.jl to change the top-level config.json, but I can't find that script

Copy link
Contributor

@cmcaine cmcaine 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 your interest! Why are you adding this exercise in particular?

@@ -0,0 +1,7 @@
# Instructions

Given a number, find the sum of all the unique multiples of particular numbers up to but not including that number.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Given a number, find the sum of all the unique multiples of particular numbers up to but not including that number.
Given a limit number and a list of other numbers, find the sum of all natural numbers less than the limit that are multiples of the other numbers.

I think the intro and blurb are unclear. Here's a potential improvement.

@test sum_of_multiples(20, [7, 13, 17]) == 51
end

@testset "factors not relatively prime" begin
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not say "coprime"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exercise, and largest-series-product, are being discussed as potential big-5 challenges for Analytical April. Both are in Python and R but not Julia. I said I'd try to add them to Julia, despite being a novice at this sort of thing, so that Erik & Jeremy have options when they make the final selection.

One way forward is that I make the Julia changes you suggest and see what you think. Are you OK with treating this as an investment for the future? I'll try to learn, then contribute enough in future to provide a return on investment.

Also, I'll go and read more about Julia, instead of submitting Python with slightly altered punctuation. I hang out with astrophysicists these days, so (as an aside) I'd be interested to see the Professor's eyebrows rise when I submit (graduate level) homework assignments in Julia instead of Python or C. This language deserves to be more widely adopted.

Apologies for dragging you into this, but I have to go through a learning process. If we get this up with just "moderate personal embarrassment, nobody died" I'll count that as a result.

Incidentally, the intro, blurb and tests are from the standard problem specifications provided by @kytrinyx, and I copied a lot of this from the Python track: it's not my wording. I agree your suggestions are mostly better (maybe not coprime - I did enough college maths to understand, but some students could be confused).

It's evening here, I'll work on this on Monday.

P.S. Don't be fooled by my Arizona location. I grew up in Prescot, about 30 miles west of you.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to apologise! I hope my tone wasn't unfriendly or unwelcoming!

Thanks for this PR. I'll open a new PR on the problem-specifications repo with some changes to the instructions. If you have any ideas about how to describe the problem then they will be very welcome.

As for this PR, let's leave the test descriptions as-is and we just need to write the approaches file and maybe change the introduction text (we can synchronise it again with problem-specifications if the community agrees on a new intro, but it's okay if this track has a slightly different text until then).

I'll write an approaches file shortly, please feel free to make changes to the example.jl or any other part :)

@SaschaMann
Copy link
Contributor

Thank you!

@@ -0,0 +1 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be blank so that we can keep track of new/changed/removed test cases in the future. You can generate it interactively with configlet sync --update, or, if you've copied all cases from problem-specs, configlet sync --tests include --exercise sum-of-multiples --update.

https://github.com/exercism/problem-specifications/#track-test-data has more info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the guidance. It should be fixed now.

Copy link
Contributor

@cmcaine cmcaine left a comment

Choose a reason for hiding this comment

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

I think this is good to go. Comments welcome on any issues with what I've written in .approaches/introduction.md.

Are there any further changes you'd like to make @colinleach ?

PR on problem-specifications: exercism/problem-specifications#2231

@colinleach
Copy link
Contributor Author

Looks great (and it's been an education!). Thanks for all your help.

@colinleach colinleach closed this Mar 13, 2023
@colinleach colinleach reopened this Mar 13, 2023
@colinleach
Copy link
Contributor Author

colinleach commented Mar 13, 2023

There were unmerged commits, I shouldn't have closed it?

@cmcaine cmcaine merged commit c376af7 into exercism:main Mar 13, 2023
@colinleach colinleach deleted the sum-of-multiples branch March 13, 2023 16:28
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