Skip to content

Add 'jigsaw-puzzle' exercise #2554

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

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

Add 'jigsaw-puzzle' exercise #2554

wants to merge 20 commits into from

Conversation

atk
Copy link

@atk atk commented Apr 7, 2025

After starting a new jigsaw puzzle last weekend, I had the idea for a new simple exercise: a helper to provide information about jigsaw puzzles from a few pieces of information.

This is the first problem-specifiation I created, so I assume there are a lot of mistakes in there.

@IsaacG
Copy link
Member

IsaacG commented Apr 9, 2025

Solving this was surprisingly difficult. It did uncover two errors in the data, though.

@atk
Copy link
Author

atk commented Apr 9, 2025

Yes, it is one of those exercises that only seem simple. I would assume a difficulty level of 6-7, depending on the language.

@atk atk marked this pull request as ready for review April 11, 2025 11:06
@atk atk requested a review from a team as a code owner April 11, 2025 11:06
atk and others added 3 commits April 13, 2025 22:53
Copy link
Member

@tasxatzial tasxatzial left a comment

Choose a reason for hiding this comment

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

@atk I haven't been following the discussion over the past few days. Is there a specific reason why there are only five test cases? I expected more tbh.

@atk
Copy link
Author

atk commented Apr 14, 2025

How many test cases would you deem sufficient?

@tasxatzial
Copy link
Member

How many test cases would you deem sufficient?

The exact number doesn't really matter, but I expect the current tests to be reasonably sufficient and cover most cases. It's a little difficult to assess whether that's the case. If you think these tests are enough, I'm fine with it — we can always add more in the future.

Copy link
Member

@tasxatzial tasxatzial left a comment

Choose a reason for hiding this comment

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

I just noticed that the aspect ratio is a float in all tests, which introduces floating-point operations. Would it be better to restrict it to integers (and let tracks add extra tests if they want to), or do we intentionally want floating-point numbers in this exercise? Maintainers?

Edit: asking this because the new test case has aspect ratio 2/3

Co-authored-by: Anastasios Chatzialexiou <[email protected]>
@atk
Copy link
Author

atk commented Apr 15, 2025

Uneven aspectRatios are pretty normal in reality. Only square puzzles or 8-pieces baby puzzles (4/2) usually come with an integer aspect ratio. As an alternative, we could use a rational number stored in an 2-element array, but then it would need to be reduced to the lowest terms in order to avoid ambiguity, which would then result in an overlap with the rational-numbers exercise - at least that was my reasoning to choose a float here.

@tasxatzial
Copy link
Member

tasxatzial commented Apr 15, 2025

I don't disagree. The question is whether floating-point manipulation is something we actually want in this exercise. It's realistic, yes, but the exercise may choose not to deal with this scenario for various reasons — making the exercise easier being one of them — while still allowing tracks to introduce extra tests with floats, if they choose to.

@tasxatzial
Copy link
Member

tasxatzial commented Apr 15, 2025

As an alternative, we could use a rational number stored in an 2-element array, but then it would need to be reduced to the lowest terms in order to avoid ambiguity, which would then result in an overlap with the rational-numbers exercise

The new test uses a truncated float, but this only results in an approximation of the columns rather than the actual integer that appears in the result. So it seems that a rational number is the only option here — or perhaps there's another solution I'm not aware of.

@atk
Copy link
Author

atk commented Apr 15, 2025

I used the output of a calculation in node.js, which should be the closest to what JSON is supposed to handle in terms of f64. I don't think using a rational number for the aspect ratio is going to simplify the exercise significantly in most languages. Yes, you do not have to handle IEEE-754 numbers and their imprecisions then, but instead you need to reduce to the lowest term, which is already covered by another exercise.

@atk
Copy link
Author

atk commented Apr 15, 2025

Using a truncated float means that you will arrive at an approximation of an integer. Rounding should work here.

@tasxatzial
Copy link
Member

Using a truncated float means that you will arrive at an approximation of an integer. Rounding should work here.

It does. I think something like this should be mentioned in a comment or similar. Let's wait until other maintainers chime in, since I'm not sure what the best way to handle this is.

@IsaacG
Copy link
Member

IsaacG commented Apr 16, 2025

I think floating point values are fine to have in the test data.

@atk
Copy link
Author

atk commented Apr 16, 2025

I think we can leave the choice to the maintainers. I have updated the documentation accordingly.

Co-authored-by: Anastasios Chatzialexiou <[email protected]>
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