Skip to content

saddle-point: use unambiguous coordinates#1429

Merged
kytrinyx merged 2 commits intoexercism:masterfrom
bitfield:saddle_point
Jan 6, 2019
Merged

saddle-point: use unambiguous coordinates#1429
kytrinyx merged 2 commits intoexercism:masterfrom
bitfield:saddle_point

Conversation

@bitfield
Copy link
Copy Markdown
Contributor

@bitfield bitfield commented Jan 4, 2019

The description for 'Saddle Point' illustrates what a saddle point is by showing an example matrix:

    0  1  2
  |---------
0 | 9  8  7
1 | 5  3  2     <--- saddle point at (1,0)
2 | 6  6  7

Because there are at least two completely reasonable coordinate systems which could apply here, (1,0) is ambiguous (does it mean "column 0, row 1", which is in fact correct, or "row 1, column 0"?)

This PR makes it unambiguous:

    0  1  2
  |---------
0 | 9  8  7
1 | 5  3  2     <--- saddle point at column 0, row 1, with value 5
2 | 6  6  7

Fixes #313.

This is also a good complement to the test data, which specifies:

      "expected": [
        {
          "row": 1,
          "column": 0
        }

@bitfield bitfield requested a review from kytrinyx January 4, 2019 18:18
@rpottsoh
Copy link
Copy Markdown
Member

rpottsoh commented Jan 4, 2019

What I see looks fine but I am wondering if everything addressed in #313 is fixed here. This comment stuck out to me when I quickly scanned #313 just now. If you decide to pursue re-basing the coordinate system to be 1 based the changes will spill over into the canonical data.

@rpottsoh
Copy link
Copy Markdown
Member

rpottsoh commented Jan 4, 2019

Matrix was recently updated to follow a more canonical notation for matrix indices. See #1387.

@bitfield
Copy link
Copy Markdown
Contributor Author

bitfield commented Jan 5, 2019

Fine. As far I can see, the issue of coordinate base is orthogonal to the issue of coordinate ordering (that's why I didn't address it in this PR). I'm happy to update it to follow the Matrix example.

Copy link
Copy Markdown
Member

@rpottsoh rpottsoh left a comment

Choose a reason for hiding this comment

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

LGTM. This probably doesn't need to be squashed when merged. Thanks for working on this @bitfield

@kytrinyx kytrinyx merged commit 00d8e30 into exercism:master Jan 6, 2019
@bitfield bitfield deleted the saddle_point branch January 6, 2019 16:48
{
"row": 3,
"column": 0
"row": 2,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice catch. Thanks

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

petertseng pushed a commit to michalfita/exercism-rust that referenced this pull request Feb 15, 2019
petertseng pushed a commit to michalfita/exercism-rust that referenced this pull request Feb 15, 2019
petertseng pushed a commit to michalfita/exercism-rust that referenced this pull request Feb 15, 2019
petertseng pushed a commit to michalfita/exercism-rust that referenced this pull request Feb 15, 2019
petertseng pushed a commit to exercism/rust that referenced this pull request Nov 2, 2019
exercism/problem-specifications#1429 which was
applied to this track in #789 uses
one-based indices in the README.

The track still uses zero-based indices in the tests. This inconsistency
is a potential source of student confusion. Prevent student confusion by
explaining the inconsistency.

Closes #791
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.

4 participants