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

alphametics: Update canonical data. Add a test case with many addends #1024

Merged
merged 1 commit into from
Dec 19, 2017

Conversation

nikamirrr
Copy link
Contributor

A test case update suggested to complement this PR:
exercism/python#1119

@rpottsoh rpottsoh changed the title Update canonical-data.json alphanumerics: Update canonical data Nov 27, 2017
@rpottsoh rpottsoh changed the title alphanumerics: Update canonical data alphametics: Update canonical data Nov 27, 2017
@rpottsoh
Copy link
Member

Exercise description.

@nikamirrr
Copy link
Contributor Author

Please, let me know if anything else needs to be fixed.
Not sure if there is any conflict with a description.

@rpottsoh
Copy link
Member

Not sure if there is any conflict with a description

It is nice to cite the exercise decription as an aid to those who may not be familiar with the exercise; it makes it just a little bit easier for someone to go reference the description quickly.

@Insti
Copy link
Contributor

Insti commented Nov 27, 2017

I'm not sure adding this test is a good idea.
I'll have to have a think about it to come up with the reasoning why.

I suspect it is because optimising a solution to solve a huge problem like this is a different problem to "simply" solving the general alphametics problem.

But having a huge test available to those who want to try it is a good thing, so maybe it does belong but with a note that it is optional?

But most test generators just take everything from canonical-data.json and stick it in tests.

@nikamirrr
Copy link
Contributor Author

In my opinion it is not very different from a 10-letter problem.
But having many repeating letters is stimulating for a more efficient way of handling them.
Other cases do not necessarily make this need obvious.
But I am OK with either decision.

@rpottsoh
Copy link
Member

rpottsoh commented Nov 27, 2017

so maybe it does belong but with a note that it is optional?

If it were to be considered as optional, perhaps it would be better suited to be included in the description instead of the canonical data?

see ISBN-Verifier description

If it is decided to update the description, then a new PR should be created for the purpose of that task.

@Insti
Copy link
Contributor

Insti commented Nov 27, 2017

Reading through the comments on exercism/python#1119 makes me more inclined to want to add it.
Especially if the complexity of the solution is approximately the same as the existing 10 letter solution.

@nikamirrr
Copy link
Contributor Author

I am OK with either decision, though I would have preferred making a contribution.
I mean, the alphametics is quite late in the python track. Permutations and string replacement may be too simple to implement and move on.
One does not even need to code a permutation, thanks to the itertools.

@Insti
Copy link
Contributor

Insti commented Nov 27, 2017

I don't want to make a call either way right now, I'd like to hear the opinion of some other people, so it might be worthwhile leaving this PR open for a week or so to give more time for people to give feedback.

The PR itself looks fine, but needs to bump the (middle) version number.

@ErikSchierboom
Copy link
Member

Although I'm fine with the added case, it does appear to make an already very challenging exercise even more challenging. I understand that this extra cases prevents brute-forcing a solution, but is that so bad? Is the goal of the exercise to have the user come up with an efficient solution, or just to come up with a solution? IMHO I think we should not be forcing people to come up with an efficient solution but also allow the brute force option, as otherwise I fear that the number of people being able to finish this exercise will be even lower than what it already is.

Note that this may not be an issue with Exercism v2, as you could define the alphametics exercise as just being an optional exercise without any unlocks.

@nikamirrr
Copy link
Contributor Author

nikamirrr commented Nov 29, 2017

Brute force runs all the cases (including the new big one) for 10s on my computer, vs 0.5s on the optimized case.
This makes me think that it will be very appropriate for push people to group the letters )
Otherwise, what is the challenge? Split the string and make a permutation call.

from itertools import permutations
from collections import defaultdict


def solve(an):
    lets = set(an) - set("=+ ")
    expr = defaultdict(int)
    nz = set()
    for s, sgn in zip(an.upper().split("=="), (1, -1)):
        for w in map(str.strip, s.split("+")):
            nz.add(w[0])
            for p, c in enumerate(reversed(w)):
                expr[c] += sgn * (10 ** p)
    lets = tuple(nz) + tuple(lets - nz)
    expr = tuple([expr[k] for k in lets])
    for digs in filter(lambda x: all(x[:len(nz)]),
                       permutations(range(10), len(lets))):
        if sum([d * m for d, m in zip(digs, expr)]) == 0:
            return dict(zip(lets, digs))
    return dict()

@petertseng
Copy link
Member

I note that in https://github.com/exercism/docs/blob/master/you-can-help/improve-exercise-metadata.md#extracting-canonical-test-data, there is a sentence that says "Performance tests should not be included in the canonical test data."

This comment must not be taken as an affirmation or rejection of the statement "The newly added test is a performance test".

If it helps, as far as I can tell, the origin of this sentence is #230 (comment).

@petertseng
Copy link
Member

If this PR is to be merged, since "update canonical data" could have many meanings, my request is that the commit message and PR title be edited to state in what way the canonical data is being updated (add a test case with many addends)

@nikamirrr nikamirrr changed the title alphametics: Update canonical data alphametics: Update canonical data. Add a test case with many addends Nov 30, 2017
@nikamirrr
Copy link
Contributor Author

Not sure what else I need to do being assigned to the PR, please, advise

@Stargator
Copy link
Contributor

@nikamirrr Since you were working on this and it's your branch, I figured I'd assign it to you. I haven't reviewed it yet, so not sure if there's anything else that needs to be done.

@nikamirrr
Copy link
Contributor Author

I don't see there is much I can do beside talking to people and closing the request.
I already provided all arguments. Someone in charge needs to make a decision.

@Stargator
Copy link
Contributor

Stargator commented Dec 17, 2017

The PR itself looks fine, but needs to bump the (middle) version number.

Agreed.

Note that this may not be an issue with Exercism v2, as you could define the alphametics exercise as just being an optional exercise without any unlocks.

It's up to the tracks to determine that and the tracks can make this a core exercise or an exercise that is unlocked at some point and essentially not blocking progress of another exercise.

So I don't think we should worry about how tracks use it, but recognize even before v2, they have a number of ways to handle it.

Copy link
Contributor

@Stargator Stargator left a comment

Choose a reason for hiding this comment

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

Since it seems this test case can be addressed via brute force, I'm fine with it. But I recognize that Exercism v2 may provide more flexibility to the user and not force them to do test cases when they may have already implemented workable code.

  • Bump the middle version number

  • Reword the commit message to contain the same message as the PR title: "alphametics: Update canonical data. Add a test case with many addends"

@rpottsoh
Copy link
Member

@nikamirrr as suggested by @Stargator I think you only need to bump the version:

 "version": "1.1.0",

Then someone will likely merge the PR.

@nikamirrr
Copy link
Contributor Author

OK. done

@Stargator
Copy link
Contributor

Per the Pull Request Guidelines:

Put the name of the exercise in the subject line of the commit. E.g. hamming: Add test case for strands of unequal length

So I still recommend doing a rebase that rewords the commit message to fit this.

@nikamirrr
Copy link
Contributor Author

Done, I guess

@@ -1,6 +1,6 @@
{
"exercise": "alphametics",
"version": "1.1.0",
"version": "1.1.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 didn't need to be bumped.

@Stargator Stargator merged commit 74faea8 into exercism:master Dec 19, 2017
@@ -96,6 +96,23 @@
"S": 6,
"T": 9
}
},
{
"description": "puzzle with ten letters and 41 addends",
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the "41 addends" part of this description. I see 199 addends. Please clarify what "41 addends" means

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, made a new PR
#1045

@petertseng
Copy link
Member

petertseng commented Dec 21, 2017

This is an interesting case. I hope all don't mind if I ask to brainstorm ways to do it fast. One thing to notice is that there are nine letters occupying the leftmost occupied column of some addend, so only one letter can be 0. So that can be assigned right away.

However, this case cripples columnwise solutions since they no longer have an advantage over brute force solutions. All nine remaining letters appear in rightmost column so columnwise solutions will evaluate 9! possibilities just like brute force solutions; so they might as well evaluate the entire problem instead of columnwise.

Unless we iterate from leftmost column first, since there are two letters in leftmost column, four in second-leftmost, and three in third-leftmost and then we are done. Sure, then we have to deal with potential values of carry-in, but it can be determined that the max carry-in is 7, 10, 15 for those three columns respectively, so we evaluate:

  • 9 * 8 * 8 possibilities, of which only 35 are possible
  • 35 * 7 * 6 * 5 * 4 * 11 possibilities, of which only 4488 are possible
  • 4488 * 3 * 2 * 1 * 16 possibilities, of which only 3568 are possible and only 1 is possible if considering all further columns to the right

And this is faster than considering all 9! possibilities without any intermediate filtering.

So the fastest I was able to get this was 1.8 seconds. Can someone help make it faster, I couldn't figure out how to do 0.5 seconds like #1024 (comment) has it.

@nikamirrr
Copy link
Contributor Author

I have another PR for the fast example solution
exercism/python#1119

@petertseng
Copy link
Member

petertseng commented Dec 21, 2017

OK, then looks like the remaining speedup there is gained by immediately terminating when finding the first solution. As opposed to continuing to find all possible solutions (verify the problem for validity to make sure it has only one possible solution not multiple). OK, guess I have to keep finding all possible solutions to verify, but solver can shortcut.

Thank you for showing!

@nikamirrr nikamirrr deleted the patch-1 branch December 21, 2017 18:25
@nikamirrr
Copy link
Contributor Author

This big problem has almost all characters in the lowest digit rank, so the initial permutation does not get reduced too much. But there are other problems where the digits are more spread through the ranks, they gain a lot of performance. 0.5s is the total time for all alphametics tests.

Another thought I had was to move the digits around the ranks.
For example, if a letter is spread among ranks 1 and 10 with multipliers of 4 and 3 for instance, move it all to rank 1 with a multiplier 44. That did not give much performance improvement.

Another idea was to spread the letters through ranks,
For example, a letter is at rand 1 with a multiplier 40, move it to rank 10 with a multiplier 4 and reduce the initial permutations.
But seems like a recursive call to check the next rank for one digit only is more expensive than to simply include it to permutations.

There are many ways to optimize this further, but I felt it is good enough for now.
Especially since this problem is almost brute-force due to many digits in the lowest ran,

nikamirrr added a commit to nikamirrr/problem-specifications that referenced this pull request Jan 4, 2018
rpottsoh pushed a commit that referenced this pull request Jan 4, 2018
* Updated one test description

The test contains 199 addends, not 41. Sorry

* Update canonical data. Add a test case with many addends #1024

Added a patch version
petertseng added a commit to exercism/haskell that referenced this pull request Jan 11, 2018
cmccandless added a commit to exercism/python that referenced this pull request Mar 5, 2018
While this test is canonical, it does not technically add additional coverage. This test serves as a test for efficiency (exercism/problem-specifications#1024 (comment)) of a solution, not completeness.

Furthermore, here are the run-times for this exercise from the [latest Travis build]() (at the time of this writing):
| Python Version | Run-time (seconds) |
| --- | --- |
| 2.7 | 3.155 |
| 3.3 | 2.461 |
| 3.4 | 3.567 |
| 3.5 | 7.270 |
| 3.6 | 0.774 |

Notice that the optimized example solution is only "fast" in 3.6.
cmccandless added a commit to exercism/python that referenced this pull request Apr 3, 2018
* alphametics: mark computationally intensive test as extra-credit

While this test is canonical, it does not technically add additional coverage. This test serves as a test for efficiency (exercism/problem-specifications#1024 (comment)) of a solution, not completeness.

Furthermore, here are the run-times for this exercise from the [latest Travis build]() (at the time of this writing):
| Python Version | Run-time (seconds) |
| --- | --- |
| 2.7 | 3.155 |
| 3.3 | 2.461 |
| 3.4 | 3.567 |
| 3.5 | 7.270 |
| 3.6 | 0.774 |

Notice that the optimized example solution is only "fast" in 3.6.

* alphametics: add to list of exercises allowed to skip tests in CI
mrcfps added a commit to mrcfps/python that referenced this pull request Apr 20, 2018
pig-latin: improve documentation (exercism#1352)

add "Running the tests" section to README template (exercism#1271)

* add "Running the tests" section to README template

* regenerate README files with new template

* rewrite Running the tests section for clarity

* -switch to a plaintext-readable format
-add link explaining py.test vs pytest

queen-attack: re-implement according to canonical data 2.1.0 (exercism#1351)

* queen-attack: re-implement according to canonical data 2.1.0

* queen-attack: rewrite to pass tests v2.1.0

* queen-attack: remove redundant tests

Rm `test_invalid_position_can_attack` since `Queen()`

grains: update tests to v1.1.0 (exercism#1357)

Fix typo and minor style issues in all READMEs (exercism#1356)

* Fix typo and style in README template

* Fix `shold` to `should`
* Fix minor style issues in `### Common pytest options`
* Add blank line after all headers

* Regenerate all READMEs

* Remove redundant periods in READMEs

add `awaiting review` exempt label to stale.yml (exercism#1364)

go-counting: adapt tests to canonical data v1.0.0 (exercism#1360)

* set correct canonical data version

* adapt tests and example solution to canonical data 1.0.0

* use assertSetEqual() consistently

sgf-parsing: implement exercise (exercism#1359)

* implement sgf-parsing

* fix import statement

* create entry in config.json

* fix __eq__ for Python2

lens-person: forego exercise (exercism#1299)

* lens-person: forego exercise

`lens-person` is specific to languages with immutable data (i.e. Haskell). This concept does not exist in Python, so the exercise should be foregone.

* remove bad comma

Implement exercise bank-account (exercism#1260)

* Implement exercise bank-account

* bank-account: generate README using configlet

* bank-account: fix typo and comments

pascals-triangle: update tests to canonical-date v1.2.0 (exercism#1164)

house: return singleton list for single verse (exercism#1354)

* house: return singleton list for single verse

RE exercism#1347 ([comment](exercism#1347 (comment)))

* house: fix example solution to pass changed tests

meetup: remove fail-safe for undefined MeetupDayException (exercism#1345)

* meetup: remove fail-safe for undefined MeetupDayException

Fixes exercism#1344

* meetup: define MeetupDayException in example solution

* meetup: fix example solution to raise correct exception

* meetup: fix flake8 violations

* meetup: add exception message

Curriculum (exercism#1355)

* select core exercises and set exercise ordering

* add missing obvious topics

* make list-ops a core exercise

* rational-numbers: increase difficulty

* unlocked_by core exercises only (exercism/configlet#61)
Ref: exercism/DEPRECATED.v2-feedback#61 (comment)

alphametics: mark computationally intensive test as extra-credit (exercism#1358)

* alphametics: mark computationally intensive test as extra-credit

While this test is canonical, it does not technically add additional coverage. This test serves as a test for efficiency (exercism/problem-specifications#1024 (comment)) of a solution, not completeness.

Furthermore, here are the run-times for this exercise from the [latest Travis build]() (at the time of this writing):
| Python Version | Run-time (seconds) |
| --- | --- |
| 2.7 | 3.155 |
| 3.3 | 2.461 |
| 3.4 | 3.567 |
| 3.5 | 7.270 |
| 3.6 | 0.774 |

Notice that the optimized example solution is only "fast" in 3.6.

* alphametics: add to list of exercises allowed to skip tests in CI

bank-account: update README using configlet (exercism#1366)

go-counting: update README to latest description (exercism#1367)

bracket-push: update tests to v1.3.0 (exercism#1369)

isbn-verifier: update tests to v2.4.0 (exercism#1373)

* Replace test case - "invalid character in isbn"
* Add test case with only 9 digits

Python "bowling" test issue. (exercism#1372)

Fixes /exercism/python/exercism#1371.

yacht: implement exercise (exercism#1368)

* yacht: implement exercise

* yacht: use enumeration of score categories

* Use enumeration instead of plain strings to represent categories
* Improve func `ns` in example solution

crypto-square: Clarify rectangular output requirement in README (exercism#1375)

scale-generator: clarify docs. (exercism#1374)

* Removed most mentions of terms that were irrelevant ("diminished interval") or undefined ("accidentals").
* Removed irrelevant table
* Some light reformatting
cmccandless pushed a commit to exercism/python that referenced this pull request Apr 20, 2018
* react: re-implement according to canonical data

* react: update tests to v1.2.0

* react: correct test method name

* react: fix example solution py2-compatibility

* `copy()` method to `[:]`
* `clear()` method to reassigning an empty list

* react: refactor callbacks into pure functions

* Merge upstream/master into react-1257

pig-latin: improve documentation (#1352)

add "Running the tests" section to README template (#1271)

* add "Running the tests" section to README template

* regenerate README files with new template

* rewrite Running the tests section for clarity

* -switch to a plaintext-readable format
-add link explaining py.test vs pytest

queen-attack: re-implement according to canonical data 2.1.0 (#1351)

* queen-attack: re-implement according to canonical data 2.1.0

* queen-attack: rewrite to pass tests v2.1.0

* queen-attack: remove redundant tests

Rm `test_invalid_position_can_attack` since `Queen()`

grains: update tests to v1.1.0 (#1357)

Fix typo and minor style issues in all READMEs (#1356)

* Fix typo and style in README template

* Fix `shold` to `should`
* Fix minor style issues in `### Common pytest options`
* Add blank line after all headers

* Regenerate all READMEs

* Remove redundant periods in READMEs

add `awaiting review` exempt label to stale.yml (#1364)

go-counting: adapt tests to canonical data v1.0.0 (#1360)

* set correct canonical data version

* adapt tests and example solution to canonical data 1.0.0

* use assertSetEqual() consistently

sgf-parsing: implement exercise (#1359)

* implement sgf-parsing

* fix import statement

* create entry in config.json

* fix __eq__ for Python2

lens-person: forego exercise (#1299)

* lens-person: forego exercise

`lens-person` is specific to languages with immutable data (i.e. Haskell). This concept does not exist in Python, so the exercise should be foregone.

* remove bad comma

Implement exercise bank-account (#1260)

* Implement exercise bank-account

* bank-account: generate README using configlet

* bank-account: fix typo and comments

pascals-triangle: update tests to canonical-date v1.2.0 (#1164)

house: return singleton list for single verse (#1354)

* house: return singleton list for single verse

RE #1347 ([comment](#1347 (comment)))

* house: fix example solution to pass changed tests

meetup: remove fail-safe for undefined MeetupDayException (#1345)

* meetup: remove fail-safe for undefined MeetupDayException

Fixes #1344

* meetup: define MeetupDayException in example solution

* meetup: fix example solution to raise correct exception

* meetup: fix flake8 violations

* meetup: add exception message

Curriculum (#1355)

* select core exercises and set exercise ordering

* add missing obvious topics

* make list-ops a core exercise

* rational-numbers: increase difficulty

* unlocked_by core exercises only (exercism/configlet#61)
Ref: exercism/DEPRECATED.v2-feedback#61 (comment)

alphametics: mark computationally intensive test as extra-credit (#1358)

* alphametics: mark computationally intensive test as extra-credit

While this test is canonical, it does not technically add additional coverage. This test serves as a test for efficiency (exercism/problem-specifications#1024 (comment)) of a solution, not completeness.

Furthermore, here are the run-times for this exercise from the [latest Travis build]() (at the time of this writing):
| Python Version | Run-time (seconds) |
| --- | --- |
| 2.7 | 3.155 |
| 3.3 | 2.461 |
| 3.4 | 3.567 |
| 3.5 | 7.270 |
| 3.6 | 0.774 |

Notice that the optimized example solution is only "fast" in 3.6.

* alphametics: add to list of exercises allowed to skip tests in CI

bank-account: update README using configlet (#1366)

go-counting: update README to latest description (#1367)

bracket-push: update tests to v1.3.0 (#1369)

isbn-verifier: update tests to v2.4.0 (#1373)

* Replace test case - "invalid character in isbn"
* Add test case with only 9 digits

Python "bowling" test issue. (#1372)

Fixes /exercism/python/#1371.

yacht: implement exercise (#1368)

* yacht: implement exercise

* yacht: use enumeration of score categories

* Use enumeration instead of plain strings to represent categories
* Improve func `ns` in example solution

crypto-square: Clarify rectangular output requirement in README (#1375)

scale-generator: clarify docs. (#1374)

* Removed most mentions of terms that were irrelevant ("diminished interval") or undefined ("accidentals").
* Removed irrelevant table
* Some light reformatting

* react: update tests to v2.0.0
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.

None yet

6 participants