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

Refine wording in Saddle Points #2413

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

Conversation

MatthijsBlom
Copy link
Contributor

As per @iHiD's request here.

@MatthijsBlom MatthijsBlom requested a review from a team as a code owner March 27, 2024 22:19
Copy link
Contributor

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Mar 27, 2024
@@ -5,7 +5,7 @@ Your task is to find the potential trees where you could build your tree house.
The data company provides the data as grids that show the heights of the trees.
The rows of the grid represent the east-west direction, and the columns represent the north-south direction.

An acceptable tree will be the largest in its row, while being the smallest in its column.
An acceptable tree will have the greatest height in its row, while having the least height in its column.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @jordancurve that there is a risk here of the very same confusion that we are trying to avoid.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't the largest or greatest height "tallest tree" and the least height being the "shortest tree", given the problem we are solving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that «the tallest tree» might not be unique. People have been frustrated by test cases with several tallest/shortest trees per row/column, because based on the instructions they expected there to be only one.

Copy link
Member

Choose a reason for hiding this comment

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

The tests should enforce the detail though, right? So any ambiguity is clarified by expressed expectation, one would think.

Copy link
Member

Choose a reason for hiding this comment

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

The whole point of this PR is to clarify this point. It is okay fort the prose to omit a detail and for the test to enforce that detail. It is less okay for the prose to imply one thing and the test to enforce something that contradicts that thing.

Copy link
Member

Choose a reason for hiding this comment

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

The tests do seem to have covered multiple "shortest tree" at least in the single column test. Or to express it in the way the test explains it "saddle points" meaning there can be more than one.
I wholly agree that the prose should not contradict, while omission is OK (not great, necessarily, but sometimes purposefully done).

I think that the detail that there may be more than one saddle point is expressed in the tests even by name.

We may see that here:

    {
      "uuid": "ee99ccd2-a1f1-4283-ad39-f8c70f0cf594",
      "description": "Can identify that saddle points in a single column matrix are those with the minimum value",
      "property": "saddlePoints",
      "input": {
        "matrix": [
          [2],
          [1],
          [4],
          [1]
        ]
      },
      "expected": [
        {
          "row": 2,
          "column": 1
        },
        {
          "row": 4,
          "column": 1
        }
      ]
    },
    {
      "uuid": "63abf709-a84b-407f-a1b3-456638689713",
      "description": "Can identify that saddle points in a single row matrix are those with the maximum value",
      "property": "saddlePoints",
      "input": {
        "matrix": [
          [2, 5, 3, 5]
        ]
      },
      "expected": [
        {
          "row": 1,
          "column": 2
        },
        {
          "row": 1,
          "column": 4
        }

@SleeplessByte SleeplessByte reopened this Mar 27, 2024
@IsaacG IsaacG mentioned this pull request Mar 29, 2024
@@ -5,7 +5,7 @@ Your task is to find the potential trees where you could build your tree house.
The data company provides the data as grids that show the heights of the trees.
The rows of the grid represent the east-west direction, and the columns represent the north-south direction.

An acceptable tree will be the largest in its row, while being the smallest in its column.
An acceptable tree will have the greatest height in its row, while having the least height in its column.
Copy link
Member

@kotp kotp Apr 3, 2024

Choose a reason for hiding this comment

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

Suggested change
An acceptable tree will have the greatest height in its row, while having the least height in its column.
An acceptable tree will be the tallest in its row, while being the shortest in its column.

Copy link
Member

Choose a reason for hiding this comment

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

@kotp "will the tallest" should that be "will be the tallest"?

Copy link
Member

Choose a reason for hiding this comment

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

@MatthijsBlom What about the above suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This suggestion comes down to

-An acceptable tree will be the largest in its row, while being the smallest in its column.
+An acceptable tree will be the tallest in its row, while being the shortest in its column.

which doesn't solve the problem this PR aims to solve.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we use the phrasing from the introduction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could. I figured the difference between introduction.md and instructions.md might be deliberate, so I kept them different.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with unifying them, especially when it turns out that the terminology is so hard to get right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use the same wording in both cases, we could also de-duplicate.

One potential advantage of using two different descriptions: these provide different perspectives on the same problem.

Copy link
Contributor Author

@MatthijsBlom MatthijsBlom Apr 5, 2024

Choose a reason for hiding this comment

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

I did not agree to applying this suggestion, but it was applied anyway.

The point of my change was to fix an inaccuracy. This now applied suggestion effectively reverts my change and reintroduces the original inaccuracy. This inaccuracy has previously irritated people, hence this PR. See the forum thread, and the other threads here.

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

I provided a "commit suggestion" block to address the tallest and shortest tree.

Co-authored-by: Erik Schierboom <[email protected]>
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

What I think might help is to have an additional example, besides the

Here is a grid that has exactly one candidate tree.

Comment on lines +10 to +11
- at least as tall as all trees to the east and west, so that you have the best possible view of the sunrises and sunsets.
- at most as tall as all trees to the north and south, to minimize the amount of tree climbing.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- at least as tall as all trees to the east and west, so that you have the best possible view of the sunrises and sunsets.
- at most as tall as all trees to the north and south, to minimize the amount of tree climbing.
- at least as tall as all the trees to the east and west, so that you have the best possible view of the sunrises and sunsets.
- at most as tall as all the trees to the north and south, to minimize the amount of tree climbing.

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