search

exercism / problem-specifications

Shared metadata for exercism exercises.
MIT License
326 stars 541 forks source link

saddle-points: Clarify it's NxN matrices or add NxM test cases #1276

Closed Alexhans closed 6 years ago

Alexhans commented 6 years ago

In the saddle_points exercise definition there's no indication that the saddle point definition should only cover NxN matrices.

That's why I added tests in my solution to cover NxM as well like so:

def test_non_square(self):
    matrix = [[3, 1, 3], [3, 2, 4]]
    self.assertEqual(saddle_points(matrix), set([(0, 2), (0, 0)]))

It has been suggested to me that I create an issue or PR to address this but I'm not entirely sure what option I should pick.

The alternative would've been to leave the tests cases as is and specify the constraints as NxN but the added complexity doesn't seem to be that much of a burden.

cmccandless commented 6 years ago

I think that specifying it as NxN would probably keep it simple which is what probably would be most beneficial for people who are doing the track.

Simplicity is good, but if a given point of complexity fits well within the scope of an exercise, then it becomes a good learning opportunity. In this case, testing for NxM matrices only slightly complicates the problem while making solutions far more robust, so I think this would be a good test case to add.

rpottsoh commented 6 years ago

I think we are saying we want an NxM test case added. The title for this issue is confusing to me. I guess we are saying that if we want to clarify that the exercise is only interested in NxN matrices then the README needs to be updated to indicate that. I am more in favor of adding a test case for NxM in which case the testdata needs to be updated.

I think this issue needs to indicate what the task should be, it should indicate the decision of direction so a concise task for a PR can be described that would close this issue. If we leave it undecided we could end up with two PRs, one for each school of thought on the issue. :-1:

Alexhans commented 6 years ago

Yes. It is confusing. I'm sorry about that.

My intention was to seek on the route to take from those two options.

I like the NxM option but thought simplicity would be desired over it.

I'll edit the issue accordingly.

rpottsoh commented 6 years ago

@Alexhans just wondering if you are working on this?

Alexhans commented 6 years ago

Hi, @rpottsoh. Sorry, I've been MIA.

Yes. I forked the problem-specifications (canonical data) and python repos and will do a PR soon. Probably in 12/14 hs when I'm back.