Skip to content

Conversation

@clechasseur
Copy link
Contributor

As discussed on the forum here, this PR adds two approaches for the sum-of-multiples exercise, along with one article discussing performance differences.

Copy link
Contributor

@senekor senekor 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 writing this up, it was clearly a lot of work and turned out great!

Apart from a few small questions / comments, I'm hesitant to include the performance article in its current state. In my opinion, the following options would make sense:

  1. Not include the performance article
  2. Leave the benchmarks in there but cut the article down to essentially: "Here are some neat benchmarks, but don't read too much into the numbers, there are simply not enough of them to draw general conclusions."
  3. Invest much time and effort to analyze performance over a broad range of inputs, allowing to detect patterns with confidence.

@MatthijsBlom
Copy link

MatthijsBlom commented Jan 17, 2024

There is a fourth option: take this exercise as an excuse for an introduction to benchmarking this kind of stuff in general.

At the end of the day, single benchmarks don't matter much. Nor do the exact big-Os of the various approaches, and neither does which approach is 'best'. In contrast, the process of finding these answers is way more interesting. Even basic understanding of this process is mostly lacking in most programmers. This exercise might offer a nice opportunity: the various approaches are very diverse, the input is straightforwardly variable, and approaches' performance characteristics are tangible.

But yeah, this is probably a lot of work.

@clechasseur
Copy link
Contributor Author

@senekor Thanks for the comments! I didn't have time to look at this again yesterday, but I'll go over your comments as soon as possible, hopefully tonight. 🙂

@clechasseur
Copy link
Contributor Author

@senekor I've tried to fix everything you pointed out. I've removed the performance article and instead added a smaller note about complexity and the need for more benchmarks, as you suggested.

If there's anything still missing, don't hesitate! 🙂

Copy link
Contributor

@senekor senekor left a comment

Choose a reason for hiding this comment

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

Great stuff! Thank you very much 😃

@senekor senekor merged commit 5d4a40e into exercism:main Jan 21, 2024

Finally, calculating the sum of the remaining unique multiples in the set is easy: we can simply call [`sum`][iterator-sum].

[^1]: There is another method available to sort a slice: [`sort_unstable`][slice-sort_unstable]. Usually, using [`sort_unstable`][slice-sort_unstable] is recommended if we do not need to keep the ordering of duplicate elements (which is our case). However, [`sort`][slice-sort] has the advantage because of its implementation. From the documentation:

Choose a reason for hiding this comment

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

Footnotes do not render properly: this document. Clicking the to/from the footnote leads to the top of the page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, thanks. I was wondering about this as well and saw that footnotes are in fact used here. Forgot to check again after deployment.

What could be the problem? maybe the missing line ---? I might ask on discord, someone's bound to know the answer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, on the site I linked, the hyperlinks to/from footnotes don't work either. This might be a general problem of the website.

Copy link
Contributor

Choose a reason for hiding this comment

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

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