Skip to content

BUG: fixed dtype alignment for array of structs in case of converting from tuple descr - #10923

Merged
ahaldane merged 1 commit into
numpy:masterfrom
ihormelnyk:master
Apr 18, 2018
Merged

BUG: fixed dtype alignment for array of structs in case of converting from tuple descr#10923
ahaldane merged 1 commit into
numpy:masterfrom
ihormelnyk:master

Conversation

@ihormelnyk

@ihormelnyk ihormelnyk commented Apr 17, 2018 •
edited by charris
Loading

Copy link
Copy Markdown

Descr of array of subtype with alignment
t = np.dtype([('a', '|i1'), ('b', [('f0', '<i2'), ('f1', '<f4')], 2)], align=True)

actual dtype:
[('a', '|i1'), ('b', [('f0', '<i2'), ('f1', '<f4')], (2,))]

expected dtype:
[('a', '|i1'), (, '|V3'), ('b', [('f0', '<i2'), (, '|V2'), ('f1', '<f4')], 2)]

Fixes #663.

ahaldane
ahaldane reviewed Apr 18, 2018
View reviewed changes
Comment thread numpy/core/src/multiarray/descriptor.c Outdated

@ahaldane ahaldane Apr 18, 2018 •
edited
Loading

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.

all the tab characters here should be four spaces. (no tabs allowed in numpy code!)

(same below)

@ahaldane

ahaldane commented Apr 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Looks good, thanks for the PR!

I have two requests, and after that I think it will be ready to merge:

  1. In the commit message body, put the exact words "Fixes Alignment doesn't work for array of structs (Trac #2211) #663". This helps us keep track, and also auto-closes the issue on github after merging.

  2. Add a unit test. This should go in the file numpy/core/tests/test_dtype.py, and you can choose an appropriate place inside. For instance, you could add a new function somewhere like the other ones there, for instance right after test_aligned_size. Or, perhaps you should just add some code at the end of test_aligned_size. Either way, I think you just need to add a few lines like in your comment above, using assert_equal to check you get the expected value.

@ihormelnyk

ihormelnyk commented Apr 18, 2018

Copy link
Copy Markdown
Author

Done.

@ihormelnyk ihormelnyk changed the title BUG: fixed dtype alignment for array of structs in case of converting from tuple descr Fixes #663: fixed dtype alignment for array of structs in case of converting from tuple descr Apr 18, 2018
ahaldane
ahaldane reviewed Apr 18, 2018
View reviewed changes
Comment thread numpy/core/tests/test_dtype.py Outdated

@ahaldane ahaldane Apr 18, 2018 •
edited
Loading

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.

these lines go over 80 characters, which is the max we want. Could you split them so they are less than 80 chars? Something like

        assert_equal(dt1.descr, [('a', '|i1'), ('', '|V3'), 
                                 ('b', [('f0', '<i2'), ('', '|V2'), 
                                        ('f1', '<f4')], (2,))])

@ahaldane

ahaldane commented Apr 18, 2018

Copy link
Copy Markdown
Member

Everything else looks good.

@ihormelnyk

ihormelnyk commented Apr 18, 2018

Copy link
Copy Markdown
Author

Fixed lines length.

@ahaldane

ahaldane commented Apr 18, 2018

Copy link
Copy Markdown
Member

Great, thanks a lot @ihormelnyk !

This was a pretty annoying bug, I'm glad it's fixed. Merging now...

@ahaldane
ahaldane merged commit 8be088a into numpy:master Apr 18, 2018
@charris charris changed the title Fixes #663: fixed dtype alignment for array of structs in case of converting from tuple descr BUG: fixed dtype alignment for array of structs in case of converting from tuple descr Jun 16, 2018
@eric-wieser eric-wieser mentioned this pull request Jan 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@ahaldane ahaldane ahaldane left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ihormelnyk @ahaldane @ihormelnyk-e