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

FIX: Handle unsigned integers in mmio #8423

Merged
merged 13 commits into from Apr 8, 2018

Conversation

@kshitij12345
Copy link
Contributor

commented Feb 14, 2018

In reference to issue #8271 , mmio.py has been modified to read and write unsigned integers as well. While reading be default the dtype will be set to uint64 to be on the safer side.

@@ -61,6 +73,18 @@ def test_simple_rectangular_integer(self):
self.check_exact([[1, 2, 3], [4, 5, 6]],
(2, 3, 6, 'array', 'integer', 'general'))

def test_simple_upper_triangle_unsigned_integer(self):

This comment has been minimized.

Copy link
@tylerjereddy

tylerjereddy Feb 20, 2018

Contributor

It may be clearer / more concise to use a parametrized test. For example, this test function is almost identical to the plain integer version. Likewise for a number of the other cases you've added. I'm not sure it is crucial, but probably a bit easier to read / more consistent with pytest style.

This comment has been minimized.

Copy link
@kshitij12345

kshitij12345 Feb 20, 2018

Author Contributor

Yeah I agree it will actually look easier to read. However I didn't want to make it too different from the current test.
Just to make sure that we are on the same page

@pytest.mark.parametrize("test_input", [[[1, 2], [3, 4]],
                                        [[2, 10], [3, 6]],
                                        [[2, 100], [3, 9]]])
def test_simple_unsigned_integer(self, test_input):
    self.check_exact(array(test_input, 'uint'),
                    (2, 2, 4, 'array', 'unsigned-integer', 'general'))

Please let me know if this suits better.

This comment has been minimized.

Copy link
@tylerjereddy

tylerjereddy Feb 21, 2018

Contributor

I mean reusing the original test instead of creating an almost identical test with a new name just because of additional type support.

Something closer to this, for example:

@pytest.mark.parametrize("typeval", ["integer", "uint"])
def test_simple_integer(self, typeval):
  self.check_exact([[1, 2], [3, 4]],
                    (2, 2, 4, 'array', typeval, 'general'))

You'd probably have to add on a dtype parameter too, if I understand correctly, for the mmio field value.

This comment has been minimized.

Copy link
@kshitij12345

kshitij12345 Feb 22, 2018

Author Contributor

The problem with that would be the original test's contain negative values of integers as well. So the unsigned has slightly different values (absolute value) from the integer test. Hence I don't think it would be wise to merge them since the test will then have to contain only positive values.

This comment has been minimized.

Copy link
@tylerjereddy

tylerjereddy Feb 22, 2018

Contributor

Looks like only a small subset of the original tests actually have negative values, so some concision / clarity can still be obtained from parametrization. For the rare cases where negative values are present in the original tests, you could also just parameterize the values or in those specific cases use separate tests.

@tylerjereddy
Copy link
Contributor

left a comment

Very nice improvements to the concision of the tests now. I added minor comments for pep8 stuff, but this is looking pretty solid to me now.

I'm not an expert on the io module so I may let others take a quick look, but otherwise +1 to merge.

FIELD_REAL = 'real'
FIELD_COMPLEX = 'complex'
FIELD_PATTERN = 'pattern'
FIELD_VALUES = (FIELD_INTEGER, FIELD_REAL, FIELD_COMPLEX, FIELD_PATTERN)
FIELD_VALUES = (FIELD_INTEGER,FIELD_UNSIGNED,FIELD_REAL, FIELD_COMPLEX, FIELD_PATTERN)

This comment has been minimized.

Copy link
@tylerjereddy

tylerjereddy Feb 26, 2018

Contributor

spaces after commas for pep8 I think


if field in (self.FIELD_INTEGER, self.FIELD_REAL):

if field in (self.FIELD_INTEGER, self.FIELD_REAL,self.FIELD_UNSIGNED):

This comment has been minimized.

Copy link
@tylerjereddy

tylerjereddy Feb 26, 2018

Contributor

space after comma I think

This comment has been minimized.

Copy link
@kshitij12345

kshitij12345 Feb 27, 2018

Author Contributor

Oh yes, missed those.. Thank You

@pv pv force-pushed the kshitij12345:master branch from 09395e8 to 656b5c3 Apr 2, 2018

@pv pv merged commit d29258d into scipy:master Apr 8, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pv pv added this to the 1.1.0 milestone Apr 8, 2018

StrahinjaLukic added a commit to StrahinjaLukic/scipy that referenced this pull request Apr 14, 2018
ENH: io: handle unsigned integers in mmio (scipy#8423)
mmio.py has been modified to read and write unsigned integers as well. 
While reading be default the dtype will be set to uint64 to be on the safer side.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.