saddle-points: Add tests for non-square matrices (NxM)#1288
saddle-points: Add tests for non-square matrices (NxM)#1288rpottsoh merged 6 commits intoexercism:masterfrom
Conversation
There's no indication in the description that non square matrices (NxM) are not covered in the provided saddle points definition. Now the tests reflect that.
| Your code should be able to provide the (possibly empty) list of all the | ||
| saddle points for any given matrix. | ||
|
|
||
| The matrix can have different a number of rows and columns (Non square). |
There was a problem hiding this comment.
Should be:
The matrix can have a different number of rows and columns (non-square).
|
Hi, @cmccandless I made the change you requested by amending the commit and forcing the push. I could've made another commit but then I probably would've had to squash the commits for a cleaner history and still have to use force again. Was this the right approach? It seems problematic in the sense that we can no longer see the file you were requesting me to change. |
|
Generally, force-pushing is not the best practice (but it does have its uses). The best approach in this case is usually to go ahead and simply make another commit. There is a "Squash and merge" option for maintainers to keep the |
rpottsoh
left a comment
rpottsoh
left a comment
There was a problem hiding this comment.
Fix the indenting. I still need to confirm that "expected" satisfies "input". In all likely hood it does, just want to be thorough.
| "row": 0, | ||
| "column": 3 | ||
| } | ||
| ] |
There was a problem hiding this comment.
A cosmetic request. Please adjust your indentation spacing to conform to the other test cases.
|
|
||
| ] | ||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
Technically this is an unrelated change. If you can undo this that would be great, or move to a separate commit. Watch out for this going forward. This won't prevent me from accepting.
| } | ||
| ] | ||
| } | ||
|
|
|
I forgot to change credentials. I'm going to have to force update with the proper ones. |
|
@Alexhans another good habit to get in to would be to create a branch within your fork that contains the changes of your PR, rather than working off your master. Don't worry about this for this PR. I am not sure what credentials you need to change. |
@rpottsoh: Yes. That sounds like a good idea. Not trying to rush things either.
The issue was committing with the wrong user.email/user.name (I have 2 aliases to set up my local config and absentmindledly used the wrong one after cloning my fork). Thanks for the patience. I'll take these things into consideration for future pull requests. |
|
Overall you have done a great job. 👍 We are only nit picking some pretty minor things. I see that you have closed this PR. Are you intending to open another with your other set of credentials? |
|
@rpottsoh: Oh, no. This one is the one I intend to merge. I've fixed the user.name and user.email on the commits already. |
|
@Alexhans is there significance to the order of expected coordinates in case For the time being I am going to assume the order is not important. If it isn't then a comment or some other modification should be made to the testdata. |
|
If the order of the points is actually relevant, the list of points ought to be sorted (probably in ascending order). |
|
The order of the coordinate pairs is opposite that of the other tests that have more than one saddle-point. There is nothing stated in the README or in the testcase about order. To me the order shouldn't be relevant. I think in most cases a solution is going to start at (0,0) when looking for valid points. As long as the number of actual points agrees with the number of expected points and each actual point matches each expected point, that should be enough.
|
I think so, yes. |
|
This conversation has been moved to #1294. |
saddle-points: update test suite to v1.3 exercism/problem-specifications#1272 exercism/problem-specifications#1288
3 participants
The saddle-points exercise made no indication that it was just for NxN matrices but tests for NxM matrices were non-existent.
I added the tests and made the description more explicit in relation to non-square matrices.
closes #1276