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

book-store: Add test that requires use of both groups of 4 and groups of 5 #1620

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sswingle
Copy link

Many of the "brute force" solutions I see do something like run a greedy solver multiple times with different maximum group sizes. This allows them to solve test cases that check for finding 2 groups of 4 instead of a group of 5 and a group of 3.

However, this wouldn't solve cases where you need to use both groups of 5 and groups of 4. For example, consider books = [1, 2, 3, 4, 5, 1, 2, 3, 4, 5, 1, 2, 3]. The cheapest price possible is achieved by splitting it into 1 group of 5 and 2 groups of 4, with cost = 800*(150.75 + 240.8) = 8120. Solutions like those described above will typically split it into 2 groups of 5 and 1 group of 3, with a cost of 800*(250.75 + 130.9) = 8160.

For further motivation and discussion see: exercism/python#2141

@cmccandless cmccandless changed the title Add test for book store that requires use of both groups of 4 and groups of 5 book-store: Add test that requires use of both groups of 4 and groups of 5 Nov 24, 2019
"description": "Two groups of four and a group of five",
"comments": ["Suggested grouping, [[1,2,3,4],[1,2,3,5],[1,2,3,4,5]]. Solutions can pass all the other tests if they just try to group without using any groups of 5. This test case breaks that since it requires both 4-groups and 5-groups."],
"input": {
"basket": [1,1,1,2,2,2,3,3,3,4,4,5,5]
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in my comment here, I would suggest shuffling the input numbers here (or adding an additional case that has the inputs shuffled), as this ensures a more robust solution.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, I've added a second test to check for that.

@Zolekah
Copy link

Zolekah commented Jan 30, 2020

Yeah that's brilliant idea to add test book store that gives in shuffled order

@yawpitch
Copy link
Contributor

I agree with adding this, but I've tended to consider an omitted test (as opposed to an included test that was manifestly wrong) as blocked by #1560 ... should this one also get a Hold label for the moment? Not that I want to add to the backlog.

Base automatically changed from master to main January 27, 2021 15:31
@ErikSchierboom
Copy link
Member

@sswingle Are you still interested in working on this PR?

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.

5 participants