new exercise alphametics #410

abo64 · 2016-10-31T06:13:30Z

of course feel free to change the place of the exercise in config.json. First I thought it makes sense to have it after sgf-parsing but then I saw its difficulty of 9 and reconsidered.
I kept the tests for multiplication and exponentiation from a former version of canconical-data.json as they make the exercise more interesting.
what annoys me is the more than 30 times worse performance than my same (I think) Scala implementation. would be very much interested to know the reason why.

rbasso

Thank you again for porting one more exercise, @abo64. 😄

In general, I would say that this implementation is very good, but the performance of the example solution is really problematic for the continuous integration:

Current rebuild time in Travis-CI: 1m52s
Rebuild time with alphametics: 5m47s

The only think that really prevents merging is the performance, but, if you want to obsess with details, I made a lot of small suggestions, none of them are really important.

Fix example's performance (under 10 seconds would be nice).
Optionally change other things.
Squash everything in one commit.

If I discover anything to improve performance I'll write here.

rbasso · 2016-11-01T04:59:31Z

exercises/alphametics/test/Tests.hs

+      where
+        assertion  = solution `shouldBe` sort <$> expected
+        solution = sort <$> solve puzzle
+


Inlining solution, it is possible to see that...

sort <$> solve puzzle `shouldBe` sort <$> expected

can be "simplified" to...

(shouldBe `on` fmap sort) (solve puzzle) expected

Which is kind of ugly, so I would create another function:

shouldMatchSolution = shouldBe `on` fmap sort assertion = solve puzzle `shouldMatchSolution` expected

But in this case, considering that it is used only one time, this is not really a problem.

I'm just pointing out because this pattern occurs a lot in the test suites and, considering that you are porting various exercises, it may be good to have it in mind.

It is a matter of style, so it's up to you to decide what you prefer.

What I really miss is a function like shouldMatchList that works with Maybe [a]...but I couldn't find or write anything like that.

rbasso · 2016-11-01T05:01:10Z

config.json

@@ -371,6 +371,12 @@
      ]
    },
    {
+      "slug": "alphametics",
+      "difficulty": 7,
+      "topics": [


I didn't solved it yet, but the positioning seems great. 👍

rbasso · 2016-11-01T05:12:09Z

exercises/alphametics/test/Tests.hs

+        assertion  = solution `shouldBe` sort <$> expected
+        solution = sort <$> solve puzzle
+
+-- Test cases adapted from `exercism/x-common/exercises/alphametics/acanonical-data.json` on 2016-10-26.


❌ acanonical-data.json
✅ canonical-data.json

rbasso · 2016-11-01T05:22:20Z

exercises/alphametics/src/Alphametics.hs

+module Alphametics (solve) where
+
+solve :: String -> Maybe [(Char, Int)]
+solve puzzle = undefined


The stub solution seems perfect.

Even if you decide to generalize the solution to accept other types (Text, Map, Set, Sequence), this is the most basic signature and should remain as it is. See #393 as an example, if you decide to try this path.

Also, including puzzle in the incomplete definition seems way better than solve = undefined. 👍

I thought so, too. One can implement a solution using a Map, say, and then easily transform to this basic type.
About including puzzle I considered

type Puzzle = String solve :: Puzzle -> Maybe [(Char, Int)] solve = undefined

but decided against it as this coding style with types might not be to everybody's liking.

rbasso · 2016-11-01T05:43:29Z

exercises/alphametics/package.yaml

+  dependencies:
+  # - foo       # List here the packages you
+  # - bar       # want to use in your solution.
+    - parsec


It is not yet documented, but now we allow a different exercise structure that separates the package.yaml provided to the user from the one used in the example. In fact, it is possible to have multiple examples using the following structure:

HINTS.md

stack.yaml

package.yaml

src/ModuleName.hs

test/Tests.hs

examples/success-foo/package.yaml

examples/success-foo/ModuleName.hs

examples/success-bar/package.yaml

examples/success-bar/ModuleName.hs

This new format could be used here to give a package.yaml without parsec to the users, avoiding inducing use of a specific library (we can still suggest it in HINTS.md).

If you like the idea, take a look at #403, because word-count has been a test-bed for trying some new things.

word-count looks good. at least some HINTS.md might be a good compromise.

rbasso · 2016-11-01T05:51:08Z

exercises/alphametics/test/Tests.hs

+import Data.Foldable     (for_)
+import Test.Hspec        (Spec, describe, it, shouldBe)
+import Test.Hspec.Runner (configFastFail, defaultConfig, hspecWith)
+import Data.List (sort)


Cosmetic suggestion

Putting it after Data.Foldable would make the imports list sorted, which is not important at all. 😄

rbasso · 2016-11-01T06:11:35Z

exercises/alphametics/test/Tests.hs

+data Case = Case { description  ::        String
+                 , puzzle       ::        String
+                 , expected     ::        Maybe[(Char, Int)]
+                 }


Cosmetic suggestion

A lot of unneeded spaces. One missing space after Maybe.

I think that this would be more readable:

data Case = Case { description :: String , puzzle :: String , expected :: Maybe [(Char, Int)] }

rbasso · 2016-11-01T06:11:39Z

exercises/alphametics/test/Tests.hs

+cases = [ Case { description  = "puzzle with three letters"
+               , puzzle       = "I + BB == ILL"
+               , expected     = Just [('I', 1), ('B', 9), ('L', 0)]
+               }


Cosmetic suggestion

I would remove one space before each = here, aligning everything with description =

rbasso · 2016-11-01T08:06:44Z

exercises/alphametics/test/Tests.hs

+               }
+        , Case { description  = "puzzle with multiplication"
+               , puzzle       = "IF * DR == DORI"
+               , expected     = Just [('I', 8), ('F', 2), ('D', 3), ('R', 9), ('O', 1)]


These two last exercises with * and ^ seem to make the exercise significantly harder, forcing people to learn a lot about parsing before even starting to solve the actual puzzle.

Most of the users give up on the track before reaching the final exercises, so I think we should focus on making the exercises more pedagogical to the less advanced students

I found that this was already discussed in exercism/problem-specifications#395, so I think that in this case we should follow x-common and remove them.

What do you think about it, @petertseng ?

thx @rbasso I will try to implement all your helpful suggestions.
I agree that the last two tests make the whole exercise harder, and we could leave them out. Or perhaps make them somehow optional by commenting them out/marking them as pending so that each student can decide for himself?
Main problem is of course the poor performance. And I still wonder why. As mentioned above it works fine in Scala. Only explanation so far I could come up with is that something causes most or all permutations to become evaluated. In Scala permutations returns an Iterator, so this might make sure that the whole computation remains lazy? I will try again to find the reason - this is an interesting problem in itself. And of course - any ideas/help appreciated.

My argument against using operations other than additions is mainly that they add the risk of inducing the student to only use a full brute force solution considering all permutations for all the letters, rather than a more considered solution considering all permutations for subsets of the letters at a time. This latter is easy to understand and implement with addition-only problems, but it's unclear how one would go about it with other operations.

Since I like to see more variety in solutions, I will prefer to see only addition tests.

(To be clear, commenting them out will also be acceptable to me, though less preferred. If that route is taken, the comment should make clear that it's not necessary to have them)

Fine then, I will remove them.
That means I will go back to the drawing board for the example solution as the current one indeed is brute force using permutations (and has this lousy performance in comparison to Scala; although I still intend to find the reason why, but later then).

It could still be the case that many students will still submit the simple brute force solution anyway. It's the first thing that comes to mind. Different people want to see different things from Exercism. So limiting to addition problems opens the door to doing something more considered, for those who want to do that. The decreased difficulty does help a bit too, both for those who only want to submit the first solution and those who want to go farther.

I realize that another way to expanding this problem is indeed to keep the operations other than addition. There are various directions to take this problem. I chose one way (and it happens to be the way that x-common chose too), but maybe others like a different way.

As for what we can do about this long runtime, maybe we can get some ideas from other tracks. https://github.com/exercism/xruby/pull/455/files#diff-8ec059eaf4c70572282d3173a6325c3aR53 the Ruby track chose to comment out the cases that had many letters since they do take a long time to solve that way. Recently as of exercism/ruby#469 the Ruby example is fast enough to solve the equations with many letters, but they still have a comment telling students it's OK not to do them: https://github.com/exercism/xruby/pull/469/files#diff-8ec059eaf4c70572282d3173a6325c3aR53 . If we decide here that the same ideas are appropriate, we could apply them.

In my view, cases that take more than a few seconds to run belong to benchmark suites. Test suites should run really fast, to give immediate feedback to the student.

I know that a few exercises in the Haskell track have test cases that take too long to run if the solution is implemented in an inefficient way, but this should be avoided.

If it is too hard to get a solution that runs in a reasonable time, maybe the exercise was designed to test performance, and that is unfortunate. In that case I would prefer to write the simplest tests that check the desired behavior and nothing more.

Performance tuning seems to be a complex topic in Haskell, and I think we should address that with optional benchmark suites. I wrote one a while ago, but never tested it enough to see if it is good (change is probably one of the best candidates for a first benchmark suite).

@petertseng I still have no clear idea what this Ruby code actually does. But I took home the idea of a partial solution and will pursue this over the weekend.
Perhaps we could also just have these additional tests in the HINTS.md?

I still have no clear idea what this Ruby code actually does.

I'll admit I'm not 100% sure either, since there's a lot going on. But the concept of partial solution seemed to make sense (I prepared a Ruby solution myself as well using the concept. Maybe I'll write a Haskell solution soon). I can explain what I would do, if you are interested: rot13(Fbyir bayl bar pbyhza ng n gvzr.) in case you don't want to get spoiled.

Perhaps we could also just have these additional tests in the HINTS.md?

Ah, the other operations? No complaints from me if that happens!

yeah, starting from the rightmost digit/column, doing the sum and then accumulating restrictions going left.
I thought that this might even work for multiplication and exponentiation.

I agree it can work with multiplication - we would have to consider more columns at a time, since in:

AB xCD --- EFG

both A*D and B*C contribute to F.

But once that detail is ironed out, it would still be doable. I have not tested it out myself to make sure, but seems that way, anyway.

(by this I just mean that you need to keep track of the column being solved and all columns to the right of it - if the problem only has addition, you only need to keep track of one column at a time plus a carry amount)

Exponentiation would thus also be doable if we just treat it as repeated multiplication, assuming there are no variable exponents.

abo64 · 2016-11-07T05:56:26Z

I have implemented this column-wise partial solution algorithm. So far it only works for addition and only up to eight letter words. Thus I moved the ten letter test as well as the tests for multiplication and exponentiation to HINTS.md.

rbasso

Now that the test suite is running really fast, I'm ready to merge it.

Change anything else if you want and them squash it and I'll merge it.

rbasso · 2016-11-07T09:23:15Z

exercises/alphametics/HINTS.md

+               , puzzle       = "PI * R ^ 2 == AREA"
+               , expected     = Just [('P', 9), ('I', 6), ('R', 7), ('A', 4), ('E', 0)]
+               }
+```


I feel that these cases pollute the HINTS.md file. Also, I don't believe anyone will copy/paste this in test/Test.hs.

I prefer them commented out in the test suite, but I will respect your decision if you prefer them here, @abo64.

Edit: I can squash it if you prefer.

abo64 · 2016-11-07T09:55:17Z

I moved the tests back to Tests.hs. Shall we then keep HINTS.md?
And I am afraid I have done some mistake while squashing? :-(

rbasso · 2016-11-07T10:53:30Z

~~You can use git rebase -i master and delete the line with the unwanted commit to fix that.~~

Edit: ignore it for a while. You case may be a little harder than that now.

abo64 · 2016-11-07T11:54:42Z

If it is too messed up: I could create a new PR with just the final changes?

rbasso · 2016-11-07T12:02:11Z

What a mess, man! 😄

Don't worry!

I'm not good at git, but I think that a I have a solution for this problem:

Type git rebase --abort, just to be sure that there is nothing ongoin.
Check that you are in the branch named exercise-alphametics.
Type git rebase -i 82ae06
Remove lines until it matches exactly what is below:

pick 7c98beb new exercise alphametics                                                                           
squash 5d75fb0 fix hlint complaint                                              
squash 39cc180 example with column-wise addition algorithm, removed problematic tests
squash dcb567e moved additional tests back to Tests.hs

Edit the commit message to something like:

alphametics: add new exercise

Push the changes to your fork: git push -f

abo64 · 2016-11-07T12:25:00Z

thanks, man! :-)

rbasso · 2016-11-07T12:31:17Z

Now you probably will want to fix your master branch. I think that something like this will work:

$ git checkout master
$ git reset --hard upstream/master
$ git pull upstream master
$ git push -f origin master

Edit: Ops. I think it is okay the way it is. Sorry!

abo64 · 2016-11-07T12:37:13Z

done.

rbasso · 2016-11-07T13:04:11Z

Thanks, @abo64! I know that this one gave you a lot of work! 👍

rbasso suggested changes Nov 1, 2016

View reviewed changes

rbasso reviewed Nov 1, 2016

View reviewed changes

rbasso approved these changes Nov 7, 2016

View reviewed changes

alphametics: add new exercise

8747367

abo64 force-pushed the exercise-alphametics branch from c3fecf8 to 8747367 Compare November 7, 2016 12:23

rbasso merged commit 0334de2 into exercism:master Nov 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new exercise alphametics #410

new exercise alphametics #410

abo64 commented Oct 31, 2016

rbasso left a comment •

edited

Loading

rbasso Nov 1, 2016 •

edited

Loading

rbasso Nov 1, 2016

rbasso Nov 1, 2016

rbasso Nov 1, 2016 •

edited

Loading

abo64 Nov 2, 2016

rbasso Nov 1, 2016 •

edited

Loading

abo64 Nov 4, 2016

rbasso Nov 1, 2016

rbasso Nov 1, 2016

rbasso Nov 1, 2016

rbasso Nov 1, 2016

abo64 Nov 2, 2016

petertseng Nov 2, 2016 •

edited

Loading

abo64 Nov 3, 2016

petertseng Nov 3, 2016 •

edited

Loading

rbasso Nov 3, 2016

abo64 Nov 4, 2016

petertseng Nov 4, 2016 •

edited

Loading

abo64 Nov 4, 2016

petertseng Nov 4, 2016 •

edited

Loading

abo64 commented Nov 7, 2016

rbasso left a comment

rbasso Nov 7, 2016 •

edited

Loading

abo64 commented Nov 7, 2016

rbasso commented Nov 7, 2016 •

edited

Loading

abo64 commented Nov 7, 2016

rbasso commented Nov 7, 2016

abo64 commented Nov 7, 2016

rbasso commented Nov 7, 2016 •

edited

Loading

abo64 commented Nov 7, 2016

rbasso commented Nov 7, 2016

new exercise alphametics #410

new exercise alphametics #410

Conversation

abo64 commented Oct 31, 2016

rbasso left a comment • edited Loading

Choose a reason for hiding this comment

rbasso Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbasso Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbasso Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cosmetic suggestion

Choose a reason for hiding this comment

Cosmetic suggestion

Choose a reason for hiding this comment

Cosmetic suggestion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

abo64 commented Nov 7, 2016

rbasso left a comment

Choose a reason for hiding this comment

rbasso Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

abo64 commented Nov 7, 2016

rbasso commented Nov 7, 2016 • edited Loading

abo64 commented Nov 7, 2016

rbasso commented Nov 7, 2016

abo64 commented Nov 7, 2016

rbasso commented Nov 7, 2016 • edited Loading

abo64 commented Nov 7, 2016

rbasso commented Nov 7, 2016

rbasso left a comment •

edited

Loading

rbasso Nov 1, 2016 •

edited

Loading

rbasso Nov 1, 2016 •

edited

Loading

rbasso Nov 1, 2016 •

edited

Loading

petertseng Nov 2, 2016 •

edited

Loading

petertseng Nov 3, 2016 •

edited

Loading

petertseng Nov 4, 2016 •

edited

Loading

petertseng Nov 4, 2016 •

edited

Loading

rbasso Nov 7, 2016 •

edited

Loading

rbasso commented Nov 7, 2016 •

edited

Loading

rbasso commented Nov 7, 2016 •

edited

Loading