Skip to content

BUG: Fix string copying for np.place#7003

Merged
jaimefrio merged 1 commit intonumpy:masterfrom
gfyoung:place_str_fix
Mar 15, 2016
Merged

BUG: Fix string copying for np.place#7003
jaimefrio merged 1 commit intonumpy:masterfrom
gfyoung:place_str_fix

Conversation

@gfyoung
Copy link
Copy Markdown
Contributor

@gfyoung gfyoung commented Jan 13, 2016

Addresses issue in #6974 with regards to string copying in np.place in which replacements strings that were smaller than their replaced elements would only partially replace the element instead of the entire element.

Previous behavior:

>>> import numpy as np
>>> arr = np.array(['12', '34'])
>>> np.place(arr, np.array([False, True]), '9')
>>> print arr
['12', '94']

New behavior:

>>> import numpy as np
>>> arr = np.array(['12', '34'])
>>> np.place(arr, np.array([False, True]), '9')
>>> print arr
['12', '9']

@gfyoung gfyoung changed the title WIP, BUG: Fix string copying for np.place BUG: Fix string copying for np.place Jan 13, 2016
@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Jan 13, 2016

Tests haven't finished yet, but I know it will have at least one test failure with test_mem_insert like in #7000. If anyone could explain that test and why it is there (I couldn't find the ticket it referred to), that would be great!

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Jan 13, 2016

In light of the discussion in #7000, I am removing test_mem_insert in this PR as well.

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Jan 13, 2016

Travis build #9122 doesn't seem to be starting up on the i386 build. Since the build is bad anyways, could somebody with the permissions kill that build? Thanks!

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Jan 13, 2016

Actually, I think something weird is going on with Travis right now. Travis build #9123 is completely stalling both on i386 and USE_DEBUG=1. Could someone kill that build (it has test failures) as well? Thanks!

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Jan 13, 2016

Yep, it's definitely something with Travis. Build #9124 is stalling as well on like half of the build machines.

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Jan 13, 2016

Did gcc just go haywire or something on Appveyor? The x86 machine with Python 2 threw a slew of errors that I can't quite understand, but the x86 machine with Python 3 passed just fine.

@gfyoung gfyoung mentioned this pull request Jan 13, 2016
Merged
@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Jan 24, 2016

Okay! Now with all of the testing issues fixed and resolved, I think this should be good to merge. If someone else could take a look at it just in case I missed anything, that would be great!

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Jan 29, 2016

Can someone take a look at this? I've been rebasing this PR onto master for some time now.

jaimefrio
jaimefrio reviewed Jan 29, 2016
View reviewed changes
numpy/core/src/multiarray/compiled_base.c Outdated
Copy link
Copy Markdown
Member

@jaimefrio jaimefrio Jan 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is redundant, already done by PyArg_ParseTupleAndKeywords when using O!.

Copy link
Copy Markdown
Contributor Author

@gfyoung gfyoung Jan 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. Done.

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Feb 4, 2016

Can someone take a look at this? Unless there are more things to refactor in the C code, I think this should be good to merge.

jaimefrio
jaimefrio reviewed Feb 4, 2016
View reviewed changes
numpy/core/src/multiarray/compiled_base.c Outdated
Copy link
Copy Markdown
Member

@jaimefrio jaimefrio Feb 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving these NULL assignments to the variable declaration may minimize the risk of them being removed inadvertently.

Copy link
Copy Markdown
Contributor Author

@gfyoung gfyoung Feb 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jaimefrio
Copy link
Copy Markdown
Member

jaimefrio commented Feb 4, 2016

Minor style nitpicks, almost there!

@gfyoung gfyoung mentioned this pull request Feb 8, 2016
Closed
@alimuldal
Copy link
Copy Markdown
Contributor

alimuldal commented Feb 8, 2016

As far as I can tell, this also addresses #7207

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Feb 10, 2016

@jaimefrio : Changes have been made, and tests are passing. If there is nothing else, should be good to merge.

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Mar 6, 2016

@jaimefrio : Any update on whether or not this PR is good to merge?

Also, if anyone else would like to take a look at this, that would be great. This PR has been ready to land for some time now.

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Mar 14, 2016

Does anyone want to take a look at this? Should be good to merge.

jaimefrio
jaimefrio reviewed Mar 15, 2016
View reviewed changes
numpy/core/src/multiarray/compiled_base.c Outdated
Copy link
Copy Markdown
Member

@jaimefrio jaimefrio Mar 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code would be a little easier to follow if you renamed your variables: max_item -> ni and ni -> nm.

Copy link
Copy Markdown
Contributor Author

@gfyoung gfyoung Mar 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jaimefrio
Copy link
Copy Markdown
Member

jaimefrio commented Mar 15, 2016

A couple of style nitpicks, plus there is a little too much vertical whitespace for my liking. But yes, it is indeed pretty much ready.

BUG: Fix string copying for np.place
9128ed5 
Fixes bug in string copying in np.place
in which replacements strings that were
smaller than their replaced elements
would only partially replace the element
instead of the entire element.

Closes gh-6974.

Addendum: this commit also appears to have
fixed issue with overflow for very large
input arrays.

Closes gh-7207.
@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Mar 15, 2016

@jaimefrio: Made style nitpicks changes (+ removed extraneous vertical whitespace) without angering either Travis or Appveyor. Should be good to merge if there is nothing else.

jaimefrio added a commit that referenced this pull request Mar 15, 2016
@jaimefrio
Merge pull request #7003 from gfyoung/place_str_fix
1429c60 
BUG: Fix string copying for np.place
@jaimefrio jaimefrio merged commit 1429c60 into numpy:master Mar 15, 2016
@jaimefrio
Copy link
Copy Markdown
Member

jaimefrio commented Mar 15, 2016

Thanks a lot for putting this together, and for your patience with the lengthy reviews, in it goes.

@gfyoung
Copy link
Copy Markdown
Contributor Author

gfyoung commented Mar 15, 2016

@jaimefrio : No worries. Compatibility issues between numpy and pandas have been keeping me quite busy for the time being. 😄

@gfyoung gfyoung deleted the place_str_fix branch March 15, 2016 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

00 - Bug component: numpy._core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gfyoung @jaimefrio @alimuldal @charris