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

matrix: add tests for column matrix #1907

Closed
wants to merge 1 commit into from
Closed

Conversation

weakish
Copy link

@weakish weakish commented Aug 17, 2019

No description provided.

@weakish weakish requested a review from a team as a code owner August 17, 2019 16:33
@cmccandless
Copy link
Contributor

Can you demonstrate a solution that passes the existing tests, but not the suggested ones? The existing test cases on a single-number matrix would seem to cover this already.

@weakish
Copy link
Author

weakish commented Aug 18, 2019

If it is parsed into a generalized nested list based data structure, depending on the design of the data structure, 1 2 3 and 1\n2\n3 may be indistinguishable, both producing a flat list [1 2 3]. But what tests assumed is [[1 2 3]] and [[1][2][3]]. Thus there will be a shape mismatched.

The right direction is making the parsing always produce nested lists. But if instead trying to cope the problem via treating parsed flat list specially, then it is possible to reach a solution that passes the existing tests, but not the suggested ones. (Because the special treatment only deals with the flat list shape mismatch issue, not the indistinguishable column matrix issue.)

numpy's array is such a data structure (roughly optimized implementation of nested list):

from io import StringIO

import numpy as np


class Matrix(object):
    def __init__(self, matrix_string):
        self.value = np.loadtxt(StringIO(matrix_string), dtype="int", ndmin=1)

    def row(self, index):
        if self.value.ndim == 1:
            return [self.value[index - 1]]
        else:
            return list(self.value[index - 1, ...])

    def column(self, index):
        if self.value.ndim == 1:
            return [self.value[index - 1]]
        else:
            return list(self.value[..., index - 1])

The above code passed all existing test, but not one of the suggested test
MatrixTest.test_extract_column_from_a_column_matrix:

>       self.assertEqual(matrix.column(1), [1, 2, 3])
E       AssertionError: Lists differ: [1] != [1, 2, 3]
E       
E       Second list contains 2 additional elements.
E       First extra element 1:
E       2
E       
E       - [1]
E       + [1, 2, 3]

@cmccandless
Copy link
Contributor

OK, I see where you're coming from. Your suggestion has called to my attention that, while the tests only deal with square matrices the description never states that the user need only handle NxN matrices. One of the following needs to occur:

  • Add a statement to the exercise description that the user's program need not worry about non-square matrices
  • Add tests for non-square matrices (such as this PR suggests)

For the Python track, at least, this exercise occurs quite early in the core progression, so my vote would be for the first option above. I'm going to create an issue in exercism/problem-specifications to address this.

@weakish, for this PR, I suggest we proceed as thus:

Since this exercise is generated, adding test cases works a little differently (we are moving towards all exercises being generated; this procedure will be the new standard in this track). Please review the contributing guide for more information.

  • The Travis build is failing because the modified tests file does not match the generator output; following convention will resolve this.
  • The contributing guide is quite new; in fact, I believe this is the first PR of this kind since I wrote it up. If there is anything unclear, creating a new issue describing the confusion would be greatly appreciated.

If you have any further questions, I will be more than happy to address them as quickly as I can!

@weakish
Copy link
Author

weakish commented Aug 18, 2019

so my vote would be for the first option above.

My vote is the same. :-)

@cmccandless
Copy link
Contributor

Since exercism/problem-specifications#1576 was merged, this test suite should be regenerated to include the added tests.

@stale
Copy link

stale bot commented Sep 23, 2019

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the abandoned label Sep 23, 2019
@stale stale bot closed this Sep 30, 2019
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.

2 participants